- User Since
- Dec 15 2017, 2:00 PM (173 w, 2 d)
Fri, Apr 9
Thu, Apr 8
As a heads up, I'm seeing segfaults on internal code as a result of this change, as well as errors in Eigen's unalignedassert.cpp test (specifically, this line asserts: https://github.com/madlib/eigen/blob/master/test/unalignedassert.cpp#L151).
FWIW, this now causes Clang to produce an error on this code, when it didn't before:
Nov 21 2020
Aha, okay. I hadn't realized that this optimization had a -fno-delete-null-pointer-checks option to disable it. I agree that since that's available there's no call for a rollback.
Nov 20 2020
So, I have bad news: This causes OpenJDK to segfault. The relevant code is here:
Nov 19 2020
As a heads-up: This is causing a lot of Clang segfaults in Google's builds with sanitizers enabled. We're working work on a reduced testcase, but wanted to let you know while we do that.
Jul 10 2020
Replying to my own question, as I was able to test this sooner than I expected: Yes, it looks like the new MSAN warnings remain after this revision. Excellent! I think that proves that this was a useful fix. :)
I'd erroneously made this comment on the revision (where I think nobody will see it) rather than here, so copying it here:
A question about this: We got a couple of dozen MSAN warnings in the Google codebase from the revision that removed these -- I'm guessing that perhaps what happened was that the undef in question was a use-of-uninitialized-value, and this optimization was hiding the use so the MSAN checks didn't trigger. Is re-enabling this going to make those MSAN warnings go away again, re-hiding this undefined behavior?
Aug 9 2019
This breaks the build with an unused-variable warning:
Jun 14 2019
Hey, so I'm seeing these tests starting to fail with llc errors. The output looks like this (after going through the output-scanning layer):
Jun 7 2019
Unfortunately, we (Google) are seeing some regressions on our internal benchmarks on x86_64 -- mostly around 2%, but in some cases a good bit more -- from this revision and some of the subsequent revisions in the series, notably r362217. Just wanted to give you a heads-up at this point -- we're still working to try to understand the underlying reasons and get a reproducing test-case, but it looks like these are going to block our internal release until they're addressed.
Jun 6 2019
For what it's worth, I was also seeing some run-time errors on x86 coming from the change that this reverts. I haven't dug into things to track them down, beyond determining that this change makes them go away again.
May 22 2019
Thanks for the quick fix for bug 41973!
Jun 9 2018
While you're doing that, maybe also fix "refer different places" in the comment on line 447 to be "refer to different places". :)
Apr 18 2018
Thanks for the summary, John. To confirm, I found two examples of bugs involving local variables, as well as the field-based examples. And, indeed, all of the false positives were in unit tests.
Apr 16 2018
A further concern about this in the general case from the reviewer of one of my test-cleanup changes: The "var = *&var" idiom is not necessarily equivalent to "var = var" in cases of user-defined types, because operator& may be overloaded.
Some further statistics, now that I've done a full cleanup on our code:
Apr 15 2018
Further note: I'm noticing that nearly all the signal is from -Wself-assign-field and all the noise is from -Wself-assign. So that would be one obvious way to make this higher-signal in what's enabled in -Wall, if that were a desire.
I have noticed two things when attempting to release LLVM with this revision internally at Google:
Dec 15 2017
We're seeing about 10% regressions in an internal benchmark as a result of this change. Still digging further, but wanted to send a heads-up.