Skip to content
This repository has been archived by the owner on Feb 4, 2020. It is now read-only.

Invalid build results (doesn't invalidate) when headers change (method names, enum order) #315

Open
nyanpasu64 opened this issue May 31, 2018 · 15 comments

Comments

@nyanpasu64
Copy link

nyanpasu64 commented May 31, 2018

I'll fill in details later... but it seems nobody reads this issue tracker anymore

@frerich
Copy link
Owner

frerich commented May 31, 2018

Don't worry, I've followed this issue tracker for the last couple of years (and some other people seem to do so, too). :-)

@nyanpasu64
Copy link
Author

  1. If I change a method's parameter types in .cpp and .h, I get link errors. The method's users were never recompiled, so the new mangled-name in the implementation != the old mangled-name in the method's users. This happens in CLion, and resolves once I delete homedir/clcache/ and rebuild. (BUG)

  2. I edit a header file holding a class { enum {} }. I comment out some enum fields. The project errors out as expected under CLion (no bug). Nothing happens under Visual Studio (BUG).


Seems like clcache is completely drunk when I'm invoking through Visual Studio 2017. I edit the header file, comment out an entire enum used in a .cpp file, and Visual Studio thinks the project's unmodified. I do a full clean and rebuild, and no syntax errors pop up.

  • Should I blame ReSharper and VAssistX? Probably not.
    • I've deleted homedir/clcache/ several times to fix stale headers. But the behavior keeps recurring, the next time I change headers.

INSTALLATION: For Visual Studio, I copied clcache.exe to cl.exe. I added the directory to PATH by copying the entire PATH prepend from "miniconda3 activate". I had to modify clcache's "cl.exe wrapper detector" so cl.exe wrapper wouldn't call itself recursively. Could my setup break high-level caching and header inclusion detection?

@frerich
Copy link
Owner

frerich commented Jun 5, 2018

It sounds like you're using the VS IDE and/or MSBuild to do your builds? Can you please try this again, but just by executing the compiler on the command line directly -- are you still able to reproduce the linker error, i.e. clcache seemingly incorrectly reusing a previously generated object file?

@nyanpasu64
Copy link
Author

nyanpasu64 commented Jun 6, 2018

I have reliably reproduced this in my own project, with CLion build system. Clone https://github.com/jimbo1qaz/0CC-FamiTracker/tree/clion and checkout the clion branch.

To reproduce the bug:

  • First build the project using clcache. (I configured CLion to use jom instead of nmake.) Then navigate to Source/SoundGen.{cpp,h}, and change void CSoundGen::GenerateVibratoTable(vibrato_t Type) to int Type, and perform a rebuild.

  • Source/CommandLineExport.cpp should get rebuilt, but isn't.

    • Line 100: theApp.GetSoundGenerator()->GenerateVibratoTable(pExportDoc->GetVibratoStyle());

Minimal test case: attempt 2.zip

  • The args variable is modeled after my CLion CMake arguments, with a few commented out.
  • Change call aconda to whatever command places the cl.exe proxy in PATH.
  • I call cl.exe once per .cpp file, like CLion CMake's Makefile, executed by nmake/jom.

First launch build.cmd (4294967295).

Then change foo.h and foo.cpp to void Foo::bar(signed x), then launch build.cmd again. (It usually works fine for me, -1).

Then undo both foo.h and foo.cpp to void Foo::bar(unsigned x). Now, (1) either the end result remains -1 despite foo.cpp being std::cout << x, or (2) I get a linking error.

@frerich
Copy link
Owner

frerich commented Jun 6, 2018

It would be great if you could reduce this setup such that CLion and CMake wouldn't be part of the setup. Having a minimal test case would allow integrating a unit test into our CI system right away.

@nyanpasu64
Copy link
Author

nyanpasu64 commented Jun 6, 2018

I posted a minimal test case, below the divider

Also all Appveyor builds are broken. Should I make a separate issue?

@frerich
Copy link
Owner

frerich commented Jun 6, 2018

Ah, sorry - I thought that too contained a CMake system. I'll try to find some time to investigate this evening when I'm out of the office.

@nyanpasu64
Copy link
Author

Did you look into the issue yet? I suspect because of the extern object in the header, its class isn't treated as a dependency when rebuilding a .cpp using that object.

@frerich
Copy link
Owner

frerich commented Jun 8, 2018

I only looked at it briefly when creating a unit test to reproduce the issue, but I didn't yet investigate why this happens (and I probably won't get around to it this weekend either). If you would like to give it a shot, feel free to!

@drussel
Copy link

drussel commented Nov 16, 2018

We are definitely seeing this too. Invoking clcache via cmake and ninja.

@dioss-Machiel
Copy link

Am I correct in that this means clcache is pretty much unusable since certain changes in source code may not be picked up?

@frerich
Copy link
Owner

frerich commented Dec 5, 2018

I would be very wary when it comes to using clcache, yes. It seems there are some conditions under which it fails to notice that a cache entry cannot be reused.

I haven't used clcache myself in about five years, so I cannot say that I noticed the same symptoms. This project is mostly in maintenance-only mode (when I get around tp it), so I hope that -- as was the case in the past -- someone steps up and 'scratches his own itch' by fixing this problem. The most important thing here would be an extension of the test suite such that we can reliably reproduce the problem in the CI builds.

@Disminder
Copy link

+1 to link errors. At least two teams on my work affected via this issue. I tested performance on our project for 100% cache hits: ~8 min without server, ~6 with it. But we can't use it :(

@Disminder
Copy link

UPD: in our case it could have been caused by using the '--disable_watching' flag.
Probably I need more research. And documentation about what this flag actually do and how it can affect.

@Artalus
Copy link

Artalus commented Dec 11, 2019

Just stumbled upon this issue and freaked out (obviously), so I decided to get @jimbo1qaz's sample a test run.

It seems that the bug appears only if CLCACHE_HARDLINK is set. If I replace the line in build.cmd with set CLCACHE_HARDLINK= and remove the clcache directory, the rebuilds happen as expected. CLCACHE_SERVER presence doesn't seem to affect the bug.

The sample can also be simplified down to:

//foo.cpp
#include <iostream>
void foo(signed x) {
	std::cout << x << std::endl;
}
//foo.h
#pragma once
void foo(signed x);
//main.cpp
#include "foo.h"
int main() {
  foo(-1);
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants