- User Since
- Jan 10 2013, 2:43 PM (236 w, 3 h)
Mon, Jul 17
Fri, Jul 14
I went ahead and landed this in r308044, given that I addressed the nits and removed the possibly contentious bits. Happy to address remaining nits in a follow-up.
Mostly done, thanks!
Thanks, all done, much better!
Thu, Jul 13
Document *, add example with multiple platforms
Tue, Jun 27
May 25 2017
May 24 2017
Either this or the reland of making allow_user_segv_handler to true broke this test on Windows:
The new test fails for me like so, at r303736:
May 10 2017
This looks good to me, thanks. Sorry about the slow turnaround. Do you have commit access? If not, I can land it for you – but it also looks like you've contributed several patches by now, so you could also ask for commit access if you don't have it yet.
May 5 2017
s/Add one/All done/
Thanks! Add one and landed in r302247.
May 4 2017
May 1 2017
Apr 24 2017
Apr 21 2017
Not sure if this is useful, but this is where I always check first.
Apr 19 2017
Looks good to me.
I was just confused again by this not working, and didn't remember this review for a while. From a user's point of view it feels like this should work, so I've gone ahead and landed this in r300692.
Apr 18 2017
I'm out today. Since lld will be a client of the library code of this tool, Rui should probably take a look if he's around this week. I'd also already add a test with just a RUN line for running the tool without parameters, just to have test coverage from day one (makes it easier to add tests in the future once the tool actually does something).
Apr 11 2017
Landed in r299952: http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20170410/189882.html
Looks good. Do you have commit access?
This breaks bootstrap builds. I put the error message in the revert commit description: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170410/444101.html
Apr 6 2017
Mar 27 2017
We have a test in chromium that loads a small so file and calls some of the functions therein. There are no allocations in the so file. The test used to run fine under tsan, now it fails with the error message you added. I'll disable the test under tsan, but maybe others out there where also calling non-allocating so's without problems before this change.
Mar 22 2017
Mar 18 2017
I think this landed in r297900.
Mar 16 2017
Ah ok, thanks for explaining. In that case, this sounds fine and I'll leave the review to zturner.
When you say "cross-compiling", you mean targeting Windows while running on non-Windows, right? How do dlls get loaded there at all?
What env vars are needed here? Reading an env file seems a bit inelegant, could we pass the values of these env vars as flags instead?
Mar 14 2017
Maybe it should have some "to suppress the warning, do X" fixit?
Looks like a cool warning. Two suggestions for more exhaustive testing, but I think this looks good.
Mar 13 2017
Mar 3 2017
http://llvm-cs.pcc.me.uk/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h/risInSystemHeader suggests that the analyzer has some plumbing for this, so I added dcoughlin as reviewer, who has touched some of those lines before. dcoughlin, as background: We're playing with running the analyzer on chromium, and we were pretty surprised that it defaults to printing diagnostics for system headers. That's different from what regular clang does, and there isn't much applications can do about diagnostics in system headers. kmarshall wrote a script to manually filter out diagnostics from system headers, but we figured it'd make more sense if the analyzer didn't emit those diagnostics in the first place -- probably by default, but maybe behind some flag. Are you familiar with the design behind the current behavior? Does it make sense to change this? Are we missing some existing flag?
Mar 1 2017
Adding incompatible extensions to MS extensions sounds like bad precedent to me. I think it'd be good if a code owner could opine on this change before it goes in.
Feb 28 2017
I'm surprised this is behind a flag. This pragma attempts to be compatible with cl.exe. Does cl.exe do this by default? If so, we should do it by default. If not, why add this as an incompatible thing to an MS extension instead of keying it off some attribute that works in non-MS mode too?
Feb 27 2017
Here's a simpler fix, suggested by rnk.
Feb 26 2017
Feb 10 2017
Jan 31 2017
Jan 13 2017
Looks great to me, thanks! Unless someone else shouts, I'll land this for you soon.
Jan 11 2017
Thanks, this is looking pretty good! From clicking around a bit on cs, do you think it's better to put the check where you have it, or is maybe http://llvm-cs.pcc.me.uk/tools/clang/lib/Analysis/UninitializedValues.cpp#36 more appropriate? I think having it where you have it means saying "we can reason about volatile vars, and we want to treat them as initialized" while the other spot means "we can't reason about volatile variables at all". I don't know which place is better (of someone else reading this knows, please speak up). If the other place makes your test fail, having the check where you have it is probably fine. Else I'd say that moving the check up is probably better.
Thanks for the test!
Jan 10 2017
Thanks! Can you add a test somewhere? grep -R W.*uninitialized test/Sema* will probably show you the existing test for this code. (Another technique: Comment some of the existing tests, run ninja check-clang, and see which tests start failing.)
Aha, looks like this did have a #pragma comment(lib when it went in (for version.lib which is actually the correct one), but takumi removed it in http://llvm.org/viewvc/llvm-project?rev=269557&view=rev
Jan 9 2017
One consequence from this that I just realized is that linking a binary depending on clang stuff with (morally):
Jan 3 2017
I'd rather we don't deviate from google style at all, but this brings us closer from where we are to that, so lgtm :-)
Dec 19 2016
Dec 18 2016
Dec 16 2016
Did you measure what this does to link times?
Dec 15 2016
Huh, I'm surprised this has a runtime component, I missed that. I thought this would be a statically-checked thing. But having this does seem useful :-)
Dec 14 2016
no dwarf or gcc mode
Dec 9 2016
(The reason that the predefines buffer doesn't override the value of __STDC_HOSTED__ from the pch is that CompilerInstance::createPCHExternalASTSource overrides the predefines buffer with a different predefines buffer suggested by the ASTReader, usually an empty one.)
update a comment
Dec 7 2016
Dec 6 2016
Dec 2 2016
Works for me too. After reading the code, I think setting (ASAN|UBSAN|MSAN)_OPTIONS=external_symbolizer_path=foo should already work too. But all the docs mention ASAN_SYMBOLIZER_PATH instead. Should we have LLVM_SYMBOLIZER_PATH in addition to the FOO_OPTIONS=external_symbolizer_path=foo flag that already exists, or is that enough? Should the docs mention that over ASAN_SYMBOLIZER_PATH and MSAN_SYMBOLIZER_PATH? Should the runtime print "use FOO_OPTIONS=external_symbolizer_path=foo instead of ASAN_SYMBOLIZER_PATH" if it's set so that it can eventually be removed?
(Unrelated, but if you're looking at memory: When I was looking at it a while ago, IIRC a surprising amount of memory was taken up by CXXBasePaths objects, and just reordering fields to pack it better made that object several bytes smaller. IIRC I accidentally reverted my local patch for this, but if you're in the mood it's probably not too hard to recreate)
Nov 28 2016
Might want to mention that this affects LLVMgold.so in the patch description.
Nov 25 2016
This breaks building on macOS:
Nov 22 2016
Nov 11 2016
(Maybe implementing real support isn't so difficult? From a quick glance, it sounds like this is a sema-only attribute?)
Nov 10 2016
Landed in r286507. Thanks!