(landed in r307867)

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!

sdy comments

arphaman comments

Thanks, all done, much better!

Document *, add example with multiple platforms

comments

r306442, thanks!

http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/2604
http://lab.llvm.org:8011/builders/clang-with-thin-lto-ubuntu/builds/3280

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:

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.

r302255, thanks!

s/Add one/All done/

Thanks! Add one and landed in r302247.

r301039, thanks.

Not sure if this is useful, but this is where I always check first.

r301032, thanks.

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.

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).

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

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.

I think this landed in r297900.

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?

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.

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?

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.

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?

r296408, thanks!

296387.

Here's a simpler fix, suggested by rnk.

(related, and why i picked rnk as reviewer: https://bugs.llvm.org/show_bug.cgi?id=17216 https://bugs.llvm.org/show_bug.cgi?id=17960)

samsonov: ping

In D28543#671953, @hfinkel wrote:

What's the motivation for this? The placement of a local volatile variable is still under the compiler's direction, and unless the address escapes, we still assume we can reason about its aliasing (and, thus, whether or not it is initialized).

r293675

Looks great to me, thanks! Unless someone else shouts, I'll land this for you soon.

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!

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

One consequence from this that I just realized is that linking a binary depending on clang stuff with (morally):

http://llvm.org/viewvc/llvm-project?view=revision&revision=290930

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 :-)

r289712

Thanks!

Did you measure what this does to link times?

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 :-)

In D27659#623405, @kubabrecka wrote:

In D27659#623368, @ygribov wrote:

There's nothing wrong with this patch, but it's a binary-compatibility issue

What is the issue with ABI?

A system library requires that bzero doesn't touch some specific register. This is true for the current system implementation of bzero, but not when using the interceptor.

reword slightly

no dwarf or gcc mode

r289228, thanks!

(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

Thanks!

comments

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)

Might want to mention that this affects LLVMgold.so in the patch description.

This breaks building on macOS:

287685, thanks!

(Maybe implementing real support isn't so difficult? From a quick glance, it sounds like this is a sema-only attribute?)

Landed in r286507. Thanks!