ClangTool::run chdir call corrupts internal Clang file path cache (?)
#515
Comments
mattmccutchen-cci
added
bug
|
I was bored, so I did the necessary research and submitted an upstream Clang bug report. I doubt we'd be able to apply a LibTooling patch to our repository anytime soon (we'd have to negotiate that with Microsoft), but I asked for ideas for a workaround. |
|
I'd like to add: My tests suggest that in a LibTooling application, the same problem of information about a file at a relative path being cached across a |
mattmccutchen-cci
changed the title
ClangTool::run chdir call corrupts internal Clang include file path cache (?)ClangTool::run chdir call corrupts internal Clang file path cache (?)
* Reverse name of 3c -disccty option to reflect what it actually does.
By default, 3c adds the -f3c-tool compiler option to disable the Checked
C type checker. 3c's -disccty option (apparently an abbreviation for
"disable Checked C type checker") made 3c _not_ pass -f3c-tool, hence
leaving the type checker _enabled_. 3C's internal flag variable,
DisableCCTypeChecker, was also backwards, though interestingly, the
`3c -help` text was correct ("Do not disable checked c type checker").
This commit renames -disccty to -enccty and renames DisableCCTypeChecker
to EnableCCTypeChecker. We take the opportunity to further improve the
`3c -help` text too.
* Completely remove the old `3c -verify` option.
Now that 3C only runs one compiler "invocation" per translation unit,
with the new diagnostic verifier implementation, callers will be able to
just use the `-Xclang -verify` compiler option. And given that we may
want to use other diagnostic verifier options (e.g., `-verify=PREFIX` or
`-verify-ignore-unexpected`), let's standardize on using the original
compiler options rather than trying to define `3c` options wrapping all
of them.
* Switch from ClangTool::buildASTs to a custom _3CASTBuilderAction.
Replace the existing ArgumentsAdjusters with modifications to the option
data structures in _3CASTBuilderAction. More will be added to
_3CASTBuilderAction in the subsequent commits.
Fixes #536.
* Canonicalize -I directory paths in _3CASTBuilderAction.
This works per translation unit, so hopefully it is a complete and
correct fix to #515 that benefits all callers of 3C.
Remove the previous crude workaround from convert_project.
Fixes #515.
* Make diagnostic verification work after the redesign of #488.
Re-enable diagnostic verification in regression tests where it still
succeeds. It remains disabled in some tests in which failures were
introduced while it was disabled; #609 is for that.
Fixes #503.
* Migrate the difftypes* tests to the diagnostic verifier.
This feels a little cleaner than manually scanning the stderr and lets
us check the diagnostics more precisely, for what that's worth. We
couldn't do this before #488 both because 3C exited before the
diagnostics could be verified and because 3C didn't support custom
verify prefixes. #488 solved the first problem, and this PR solves the
second.
* Document diagnostic verification in 3C's CONTRIBUTING.md.
As part of the change to expand macros before running 3C, I tried to change
convert_projectso that instead of (1) passing an adjusted version of the union of all compiler options seen in the compilation database to 3C via-extra-arg-before, it (2) lets 3C read the options directly from the compilation database. This is because approach (1) may be wrong if different translation units have different compiler options, and I was more concerned about this asconvert_projectstarted to have more direct interaction with the preprocessor. Importantly, the adjustment in (1) included expanding relative paths in-Ioptions to absolute paths based on the working directory of the translation unit carrying the options.Unfortunately, this change seemed to cause our icecast benchmark to trigger a bug in Clang LibTooling. The symptom looks like this:
My rough theory is as follows: Clang has a cache where the first time it sees
#include STR(whereSTRis of the form<PATH>or"PATH"), it searches the include path for the first matching file and caches the path at which it found the file (to a first approximation, the concatenation of the-Idirectory withPATH). If Clang later sees#include STRagain, it tries to open the cached path directly and raises a fatal error (seen above) if it fails. The problem arises when the cached path is relative, which can occur if the directory path specified via-Iwas relative.ClangTool::runiterates over the specified translation units, and for each one, it does achdirto the working directory specified in the compilation database but (apparently) does not invalidate the cache. Consequently, if different translation units have different working directories, the preprocessor may try and fail to open a cached relative path because the working directory is different than it was when the path was added to the cache, when instead the preprocessor should do the include search over. Surprisingly, #488 did not fix the problem becauseClangTool::buildASTsstill callsClangTool::runinternally (!).Here is the original benchmark workflow run in which the problem appeared (though the logs will probably expire from GitHub soon). It should be possible to reproduce the problem by re-running that revision of the
preprocess-before-conversionworkflow (mwhicks1/3c-actions@7651529) on the corresponding revision of thepreprocess-before-conversionbranch of this repository (c113b1d). We could probably construct a smaller test case with a compilation database with two entries (and presumably that's what we would do if we wanted to add a regression test for the problem to 3C), but I don't want to take the time to do that now.In a web search, I found a few reports of similar-looking problems (1, 2), but it doesn't appear that anyone has tracked down the details and formally reported the bug in the Clang bug database. We could do so if we wish, assuming (as Mike pointed out) that it still reproduces on the latest
mainbranch of the LLVM monorepo; to test that, we'd probably want to write a trivial LibTooling-based app rather than try to upgrade the wholecheckedc-clangcodebase to Clangmainjust for this test.Ultimately, we'll probably want to fix or work around this problem somehow so that end users get correct behavior when running 3C on a compilation database like that of icecast. For now, I'm planning to work around the problem in
convert_projectby restoring the legacy behavior of passing-extra-arg-beforeto 3C, but only for the absolute versions of-Ioptions. Since we use-extra-arg-before, this will ensure that every included file is found via an absolute-Idirectory before we reach the relative ones in the compilation database, so the cached path will be absolute, avoiding the problem. In principle, this could be wrong if different translation units have different sets of resolved-Idirectories: if we apply the union of the-Idirectories to all translation units, then a translation unit could use a file from an-Idirectory that was not supposed to be active for that translation unit, when it was intended to use a file from a later-Idirectory instead. However, I don't believe this happens in any of our current benchmarks.The text was updated successfully, but these errors were encountered: