Differential D17993
[CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments. Authored by CJ-Johnson on Mar 9 2016, 7:32 AM.
Details Changes in this diff
Diff Detail Event TimelineComment Actions I'm *really* nervous about doing anything with -f(no-)delete-null-pointer-checks that makes it look like we support this feature without actually supporting it in the backend. In computePointerICmp in InstructionSimplify.cpp, we have: // A non-null pointer is not equal to a null pointer.
if (llvm::isKnownNonNull(LHS, TLI) && isa<ConstantPointerNull>(RHS) &&
(Pred == CmpInst::ICMP_EQ || Pred == CmpInst::ICMP_NE))
return ConstantInt::get(GetCompareTy(LHS),
!CmpInst::isTrueWhenEqual(Pred));I'd much rather have the frontend add an attribute (and I think it must be an attribute because of how this needs to work in LTO) that disables this, and any other necessary checks. Comment Actions Hal, I think you're talking about a slightly different thing. This patch is adding an assumption that C++ this pointers are non-null, but only when -fno-delete-null-pointer-checks is not passed. The flag is therefore at least somewhat functional. (I would argue that it should also cover the other cases where we add non-null assumptions, e.g. with reference arguments. It's less clear whether it should change the code-generation of, say, non-trivial l-value derived-to-base casts, where there's a null check that's implicitly elided in the frontend.) If you're saying that GCC's interpretation of this flag is more aggressive than that — e.g. if it also changes how aggressively the optimizer will decide that a pointer is non-null due to (say) it having been previously used as a load/store argument, as in the infamous Linux kernel bug that GCC got heat over — then I agree that we should probably avoid saying that we support the attribute until we find a way to emulate that. That, I assume, would have to be an attribute on the Function that weakens that analysis. I hope GCC's interpretation of this flag doesn't completely disable null-check elimination, e.g. when the pointer is obviously the address of a local or global variable.
Comment Actions If we're not going to fully implement "fdelete-null-pointer-checks" we shouldn't claim to... I'm really worried about us accepting that flag and not actually honoring it. However, I *do* think this should be guarded by a flag, and it should be specific to the 'this' pointer. And I'm also sufficiently terrified of this that I think the flag should be off to start with so that folks can find out how bad this is really going to be... Comment Actions GCC 6 is already doing this and people are already annotating their builds with -fno-delete-null-pointer-checks. Putting it under a different flag will break compatibility there :( Comment Actions Well, no, we can do it under a different flag and just ensure that -fno-delete-null-pointer-checks *also* has the effect of disabling the optimization. But you are not inspiring to disagree with Chandler about whether this optimization should be enabled by default. Comment Actions I agree with you, but I don't really understand why this particular use of nonnull is scary. Do you really think we have people calling member functions on null pointers on purpose and checking for a null this pointer in their code? Comment Actions Yes, we saw an astonishing number of instances of this when turning on the warning. I don't have any real sympathy for the pattern or principle of the code -- it is *really* bad. But at the same time, I do have sympathy for the users of Clang who often inherited that code and would have to make substantial plans to fix all of it before this kind of thing could be rolled out. Comment Actions We certainly can use a separate flag to control just this use of nonnull (and you're right, we should probably also have a corresponding way to turn off the nonnull reference assumptions). I'm just very afraid of incomplete implementations of -fno-delete-null-pointer-checks (seems like we're all on the same page about this now).
Yes, this is my understanding, and I think a function attribute would be appropriate here.
I don't know; but we should certainly find out. Comment Actions Interesting. Then, indeed, we at least need a flag. Not sure we need nonnull this disabled by default, however. Comment Actions Not to hijack this review, but would it not make sense to do something similar with the 'this' pointer and the 'dereferenceable' attribute? Granted, figuring out the size to use is trickier, but I believe this would enable LICM e.g. to hoist member variable loads out of loops in more cases. Comment Actions In the cases where we can emit nonnull, I agree that we should be able to safely use dereferenceable for the non-virtual data size of of the type. I'm not too concerned about people calling methods on entirely the wrong type, especially to the relatively small degree that deferenceable can actually cause miscompiles. Getting back to the original discussion, I understand the desire to be strict about the language standard by default, but that's a lot easier when we're talking about static properties of source than when we're talking about dynamic properties of programs. Nobody is well-served by us introducing a compiler option that will only have to be widely disabled, especially if the only way to disable it also disables a much broader class of optimization, as appears to be the case in GCC. I propose embracing a three-tier system. There are a lot of optimizations (current and future) that are only made legal by strict interpretation of the language standard. Some of those optimizations are reasonable for us to turn on by default; others are more treacherous. We should recommend that programmers use of three different options: -flanguage-optimization=strict, -flanguage-optimization=lax, or the default (-flanguage-optimization=cautious?). Each will imply a different set of defaults for each of the language-informed optimizations. Programmers who feel comfortable that their programs are conforming can opt in to strict optimization; programmers who are very worried about it can opt out of even our more conservative optimizations (like -fno-strict-aliasing). Comment Actions After chatting with bkramer, I'm working on rebasing this diff so that it can be landed. Comment Actions Rebasing on head, removing flag changes since that was added in https://reviews.llvm.org/D47894 and fixing broken tests Comment Actions My apologies, I am new to LLVM contribution. What is the best way to do that such that it squashes all of my local git commits?
Comment Actions I think this is the wrong patch now. @xbolva00 posted a command that works if git show HEAD shows the commit you want to upload. If not, replace HEAD with the commit hash.
Comment Actions Overall this looks sane to me. Don't know who wants to accept this. @rjmccall @lebedev.ri @aaron.ballman @rsmith
Comment Actions A lot of the test changes here seem to be over-constraining the existing tests. Tests that don't care about nonnull / dereferenceable this pointers should generally not be checking for that. Instead of looking for nonnull dereferenceable(...), I'd prefer to see {{[^,]*}} or similar to skip any parameter attributes. I would also like to see some tests that directly cover the new functionality: specifically, tests that check that the argument to dereferenceable is correct, including cases such as classes with virtual bases, virtual function calls, calls through pointers to member functions, and a check that we don't emit the attributes under -fno-delete-null-pointer-checks.
Comment Actions The new tests can be found in this-nonnull.cpp: https://reviews.llvm.org/differential/changeset/?ref=2242268 Comment Actions Thanks! We should also have a test for the behavior when targeting the MS ABI, where we sometimes don't emit the nonnull dereferenceable because the "this" pointer might actually point outside the object, but otherwise I think this is ready to go. Please can you also put together a patch for the release notes? This seems worth mentioning there.
Comment Actions So, I have bad news: This causes OpenJDK to segfault. The relevant code is here: void Arena::destruct_contents() {
if (UseMallocOnly && _first != NULL) {
char* end = _first->next() ? _first->top() : _hwm;
free_malloced_objects(_first, _first->bottom(), end, _hwm);
}
// reset size before chop to avoid a rare racing condition
// that can have total arena memory exceed total chunk memory
set_size_in_bytes(0);
_first->chop();
reset();
}I've also seen a segfault in Verilator that root-causes to this patch, though I haven't yet tracked that down to the source code. I hate to say it, but is this a significant enough problem to call for a (temporary, I hope) rollback? Comment Actions I don't see why this would be enough for a rollback, jdk is supposed to build with -fno-delete-null-pointer-checks, which disables this optimization: Is the build system not setting this when using Clang? Comment Actions In that code there is a comment:
So you should really fix UB... Comment Actions 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. (I'll also make sure upstream bugs are filed for the cases in OpenJDK and Verilator that I know about, though I imagine those are likely to take a little while to make it out into the ecosystem.) | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please include HelpText, and it's probably worth talking about this in the actual docs.