Fix according to the discussion here: D53280
Details
- Reviewers
george.karpenkov Szelethus - Commits
- rG46debda1c7fb: Revert "[driver] [analyzer] Fix a backward compatibility issue after r348038."
rGc93968a5e850: [driver] [analyzer] Fix redundant test output.
rG59166506bc7e: [driver] [analyzer] Fix buildbots after r349824.
rGd001380a69cd: [driver] [analyzer] Fix a backward compatibility issue after r348038.
rC349843: Revert "[driver] [analyzer] Fix a backward compatibility issue after r348038."
rL349843: Revert "[driver] [analyzer] Fix a backward compatibility issue after r348038."
rC349835: [driver] [analyzer] Fix redundant test output.
rL349835: [driver] [analyzer] Fix redundant test output.
rC349828: [driver] [analyzer] Fix buildbots after r349824.
rL349828: [driver] [analyzer] Fix buildbots after r349824.
rC349824: [driver] [analyzer] Fix a backward compatibility issue after r348038.
rL349824: [driver] [analyzer] Fix a backward compatibility issue after r348038.
Diff Detail
Event Timeline
Ugh. No, it doesn't work. It seems that stuff after -Xclang is not included in Args (even at the beginning of the function, when no arguments are consumed), and it doesn't make it into CmdArgs (yet?).
Test passes because -analyzer-config is a sub-string of the file name. If you rename the test, it stops working (:
I'll be debugging this a bit more on my end, but help is welcome. And in any case, thanks a lot for looking into this!~ I'm even more surprised how hard it is to come up with a sensible solution to our weird problem.
test/Analysis/invalid-analyzer-config-value.c | ||
---|---|---|
72 | This run-line *does* have -analyze specified, because that's what --analyze expands to. I guess the test that we need would be along the lines of clang -Xclang -analyzer-config ... without --analyze. |
These arguments seem to be available during the top-level call, Driver::BuildCompilation. I guess we need to stick this somewhere in between.
test/Analysis/invalid-analyzer-config-value.c | ||
---|---|---|
72 | Uhhhh sorry about that. |
lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
3697 | When I dump Args for the following invocation, I get this: clang -c dummie.cpp -Xclang -analyzer-config -Xclang blahblah=haha * < Opt:<FlagClass Prefixes:["-"] Name:"c" Group:<GroupClass Name:"<action group>"> > Index:0 Values: []> * < Opt:<InputClass Prefixes:[] Name:"<input>"> Index:1 Values: ['test/Analysis/cxx-uninitialized-object.cpp']> * < Opt:<SeparateClass Prefixes:["-"] Name:"Xclang" Group:<GroupClass Name:"<CompileOnly group>"> > Index:2 Values: ['-analyzer-config']> * < Opt:<SeparateClass Prefixes:["-"] Name:"Xclang" Group:<GroupClass Name:"<CompileOnly group>"> > Index:4 Values: ['blahblah=haha']> so there should be a way to retrieve it. For Args.getArgString(/*index*/3) it also prints "-analyzer-config" -- but I'm still looking for a "proper" way. |
I'm still looking for a sensible solution, but I'll at least share a patch that actually works.
test/Analysis/invalid-analyzer-config-value.c | ||
---|---|---|
72 | I checked this against a completely different file, and it works. |
It works! :D
I guess, please commit this if you don't find anything better.
test/Analysis/invalid-analyzer-config-value.c | ||
---|---|---|
72 | Given that we stepped on this, i guess it's still worth it to rename the test file. |
lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
3700 | Needs an LLVM-style loop!~ :) |
lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
3700 | Shouldn't this be under else- for the previous branch? Otherwise it seems that the option would be added twice. |
@Szelethus: Ok if we commit this real quick and then see if there's a better solution?
Sure! I'm on my phone, and will be for a little while, can you commit on my behalf?
lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
3700 | I originally deleted that line, but I lost in somewhere :/ |
Re-remove the first argument append.
Add a test in which file name doesn't match our search.
Thanks! Sorry about being a little slow with this, I'm sadly busier than I expected, but I'll definitely think about a nicer solution.
Hmm, this http://lab.llvm.org:8011/builders/clang-with-thin-lto-ubuntu/builds/14563/steps/test-stage2-compiler/logs/stdio also looks like ours. I guess i'll revert cause i've no idea what causes that.
Nope, we crash on index lookup. It seems that Args.size() is larger than the small vector that we're allowed to access via .getArgStrings(). I tried Args.getNumInputArgStrings() and it seems better.
George committed this as rC349863. Before that happened, we kinda figured out how it works: -analyzer-config is not a flag, it's an argument of the -Xclang flag, so we should look for -Xclang and see what value does the argument have.
Now, there's also -Xanalyzer. But -Xanalyzer is automatically not consumed during compilation, so any -analyzer-config flags passed through it would be ignored during compilation.
However, --analyze -Xanalyzer -analyzer-config ... is now broken :/
When I dump Args for the following invocation, I get this:
clang -c dummie.cpp -Xclang -analyzer-config -Xclang blahblah=haha
so there should be a way to retrieve it. For Args.getArgString(/*index*/3) it also prints "-analyzer-config" -- but I'm still looking for a "proper" way.