Page MenuHomePhabricator

[CodeGen] Apply 'nonnull' to 'this' pointer arguments.
Needs RevisionPublic

Authored by CJ-Johnson on Mar 9 2016, 7:32 AM.

Details

Diff Detail

Event Timeline

bkramer updated this revision to Diff 50136.Mar 9 2016, 7:32 AM
bkramer retitled this revision from to [CodeGen] Apply 'nonnull' to 'this' pointer arguments..
bkramer updated this object.
bkramer added reviewers: rsmith, rjmccall.
bkramer added a subscriber: cfe-commits.
hfinkel added a subscriber: hfinkel.Mar 9 2016, 8:01 AM

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.

rjmccall edited edge metadata.Mar 9 2016, 9:28 AM

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.

include/clang/Driver/Options.td
1157 ↗(On Diff #50136)

Please include HelpText, and it's probably worth talking about this in the actual docs.

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

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

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.

Er, sorry. You are not inspiring *me* to disagree with Chandler.

rsmith edited edge metadata.Mar 9 2016, 4:40 PM

-f(no-)strict-nonnull-this maybe?

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

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?

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

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?

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.

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

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

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.

Yes, this is my understanding, and I think a function attribute would be appropriate here.

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.

I don't know; but we should certainly find out.

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

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?

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.

Interesting. Then, indeed, we at least need a flag. Not sure we need nonnull this disabled by default, however.

gberry added a subscriber: gberry.Apr 11 2016, 2:38 PM

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.

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

CJ-Johnson edited the summary of this revision. (Show Details)Fri, Oct 9, 7:06 AM
CJ-Johnson added inline comments.
include/clang/Driver/Options.td
1157 ↗(On Diff #50136)

This flag has since been added via D47894 😃

After chatting with bkramer, I'm working on rebasing this diff so that it can be landed.

CJ-Johnson commandeered this revision.Mon, Oct 12, 10:53 AM
CJ-Johnson added a reviewer: bkramer.

The patch is ready! Commandeering this change :)

CJ-Johnson edited the summary of this revision. (Show Details)

Rebasing on head, removing flag changes since that was added in https://reviews.llvm.org/D47894 and fixing broken tests

Can you please upload again with full context?

Can you please upload again with full context?

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?

Can you please upload again with full context?

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?

You can use git show HEAD -U999999 > mypatch.patch (-U999999 for context)

https://llvm.org/docs/Phabricator.html

Update diff with full context

arsenm added inline comments.Mon, Oct 12, 11:22 AM
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
1082 ↗(On Diff #297645)

How are these changes related?

Update with full diff context

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.

CJ-Johnson added inline comments.Mon, Oct 12, 11:24 AM
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
1082 ↗(On Diff #297645)

This is a mistake. I did not add this and I'm not sure why it is being included in the change.

CJ-Johnson added inline comments.Mon, Oct 12, 11:29 AM
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
1082 ↗(On Diff #297645)

I believe this has been fixed now. Can you confirm?

jdoerfert added inline comments.Mon, Oct 12, 11:29 AM
clang/lib/CodeGen/CGCall.cpp
2174

Even if null pointer is valid we should place dereferenceable.

We also could never place nonnull and let the middle-end make the dereferenceable -> nonnull deduction, though I don't see why we can't just add nonnull here.

Apply dereferenceable even if null is a valid address

CJ-Johnson marked an inline comment as done.Mon, Oct 12, 11:53 AM
CJ-Johnson added inline comments.Mon, Oct 12, 11:57 AM
clang/lib/CodeGen/CGCall.cpp
2174

I re-ran ninja check after making this fix and addressed the newly-affected tests. So the patch is fully up to date :)

CJ-Johnson marked an inline comment as done.Mon, Oct 12, 12:04 PM

Overall this looks sane to me. Don't know who wants to accept this. @rjmccall @lebedev.ri @aaron.ballman @rsmith

clang/lib/CodeGen/CGCall.cpp
2165

I'm unsure why this assertion has to hold and more importantly why we need it. @arsenm do you?

rsmith requested changes to this revision.Mon, Oct 12, 3:03 PM

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.

clang/lib/CodeGen/CGCall.cpp
2174

The LLVM LangRef says that in address space 0, dereferenceable implies nonnull, so I don't think we can emit dereferenceable in NullPointerIsValid mode, and we'd need to use dereferenceable_or_null instead. (Perhaps the LangRef is wrong, though, and the null_pointer_is_valid function attribute overrides that determination.)

clang/test/CodeGenCXX/array-default-argument.cpp
22–24

This has changed the meaning of the test. Previously we were checking the argument bound in the first call is also passed to the second and third calls; now we're not checking the call arguments at all, because we re-bind TEMPORARY in each CHECK line.

clang/test/CodeGenCXX/microsoft-abi-multiple-nonvirtual-inheritance.cpp
126

Why does this call get nonnull but not dereferenceable? Seems like we should be able to use at least dereferenceable(sizeof(Right)) here -- but I think we could actually be more aggressive and pass dereferenceable(sizeof(ChildOverride) - offsetof(ChildOverride, <Right base class>)).

This revision now requires changes to proceed.Mon, Oct 12, 3:03 PM
jdoerfert added inline comments.Mon, Oct 12, 4:56 PM
clang/lib/CodeGen/CGCall.cpp
2174

(Perhaps the LangRef is wrong, though, and the null_pointer_is_valid function attribute overrides that determination.)

This is the case. IMHO, dereferenceable has to be correct here, regardless of the mode. You could access the object in the function entry, which we would use to deduce dereferenceable, and if the NullPointerIsValid is not translated to the function attribute also to nonnull.

rsmith added inline comments.Wed, Oct 14, 11:46 AM
clang/lib/CodeGen/CGCall.cpp
2165

Even if the this pointer is always an address-space-0 pointer for now, it would be more consistent with how we handle Attribute::NonNull elsewhere to skip adding the NonNull attribute for non-address-space-0 pointers rather than asserting here.

2174

OK, if the LangRef is wrong about this, then I agree we should emit dereferenceable unconditionally. Thanks for clarifying.