[CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments.

[CJ-Johnson]

Summary

Changes in this diff

• Adds 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments
• Gates 'nonnull' on -f(no-)delete-null-pointer-checks
• Introduces this-nonnull.cpp and microsoft-abi-this-nullable.cpp tests to explicitly test the behavior of this change
• Refactors hundreds of over-constrained clang tests to permit these attributes, where needed
• Updates Clang12 patch notes mentioning this change

[hfinkel]

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]

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.

[chandlerc]

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

[bkramer]

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

[rjmccall]

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.

[rjmccall]

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

[rsmith]

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

[hfinkel]

In D17993#370793, @chandlerc wrote:

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?

[chandlerc]

In D17993#371454, @hfinkel wrote:

In D17993#370793, @chandlerc wrote:

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.

[hfinkel]

In D17993#370790, @rjmccall wrote:

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.

[hfinkel]

In D17993#371459, @chandlerc wrote:

In D17993#371454, @hfinkel wrote:

In D17993#370793, @chandlerc wrote:

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]

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.

[rjmccall]

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]

include/clang/Driver/Options.td

1157 ↗

(On Diff #50136)

This flag has since been added via D47894 😃

[CJ-Johnson]

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

[CJ-Johnson]

The patch is ready! Commandeering this change :)

[CJ-Johnson]

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

[jdoerfert]

Can you please upload again with full context?

[CJ-Johnson]

In D17993#2325610, @jdoerfert wrote:

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?

[xbolva00]

In D17993#2325616, @CJ-Johnson wrote:

In D17993#2325610, @jdoerfert wrote:

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

[CJ-Johnson]

Update diff with full context

[arsenm]

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

1082 ↗

(On Diff #297645)

How are these changes related?

[CJ-Johnson]

Update with full diff context

[jdoerfert]

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]

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]

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

1082 ↗

(On Diff #297645)

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

[jdoerfert]

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.

[CJ-Johnson]

Apply dereferenceable even if null is a valid address

[CJ-Johnson]

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

[jdoerfert]

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]

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

[jdoerfert]

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]

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.

[CJ-Johnson]

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

126

Fixed! Added a check for isVoidPointerType() :)

[CJ-Johnson]

The new tests can be found in this-nonnull.cpp: https://reviews.llvm.org/differential/changeset/?ref=2242268

[rsmith]

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.

clang/test/CodeGenCXX/this-nonnull.cpp

2–3

This test needs a -triple or it will fail on (at least) non-Itanium hosts.

[CJ-Johnson]

In D17993#2367447, @rsmith wrote:

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.

Done and done. Thanks!

[rsmith]

clang/test/CodeGenCXX/this-nonnull.cpp

2–3

Please use a specific triple here (eg x86_64-linux-gnu); right now this test would fail on Itanium targets where sizeof(Struct) is not exactly 12 (which is not guaranteed -- int might not be 32 bits wide on all Itanium targets).

[CJ-Johnson]

clang/test/CodeGenCXX/this-nonnull.cpp

2–3

This has now been switched to -triple=x86_64-linux-gnu :)

[jdoerfert]

Please modify the commit subject and add a proper message.

[CJ-Johnson]

In D17993#2398337, @jdoerfert wrote:

Please modify the commit subject and add a proper message.

Thank you for the reminder! It slipped my mind.

Fixed :)

[jdoerfert]

LGTM, thx!

[arichardson]

clang/lib/CodeGen/CGCall.cpp

2169

Isn't the this pointer also nonnull in other address spaces?

In our CHERI fork we use AS200 for the this pointer and would quite like to have the nonnull attribute.
I can obviously change this line locally when I next merge from upstream, but I would like to avoid diffs and it seems to me like this restriction is unnecessary.

[jdoerfert]

clang/lib/CodeGen/CGCall.cpp

2169

I also think NullPointerIsValid is sufficient.

[rsmith]

clang/lib/CodeGen/CGCall.cpp

2169

It's my understanding that:

• The LLVM null value in any address space is the all-zero-bits value.
• In address space zero, the null value does not correspond to addressable memory, but this is not assumed to hold in other address spaces.
• An address-space-zero null value that is addressspacecast to a different address space might not be the null in the target address space.
• The nonnull attribute implies that the pointer value is not the null value.
• A null pointer in the frontend in a non-zero address space corresponds to the value produced by an addressspacecast of an address-space-zero null value to the target address space.

That being the case, there is simply no connection between the C and C++ notion of a null pointer and a null LLVM pointer value in a non-zero address space in general, so it is not correct to use the nonnull attribute in a non-zero address space in general. Only if we know that a C++ null pointer is actually represented by the LLVM null value in the corresponding address space can we use the nonnull attribute to expose that fact to LLVM. And we do not know that in general.

[rjmccall]

clang/lib/CodeGen/CGCall.cpp

2169

I think all of this is correct except that the frontend does know what the bit-pattern of the null pointer is in any particular language-level address space, and it knows what the language-level address space of this is. So we should be able to ask whether the null value in the this address space is the all-zero value and use that to decide whether to apply nonnull.

[jdoerfert]

clang/lib/CodeGen/CGCall.cpp

2169

Hm, I think the problem is that we don't couple NullPointerIsValid with the address space. As I said before. In LLVM-IR, if we don't have the null-pointer-is-valid property, then "memory access" implies dereferenceable implies nonnull. Now, we usually assume non-0 address spaces to have the above property, but that should be decoupled. The query if "null is valid" should take the function and the AS, as it does conceptually in LLVM-core, and then decide if null-pointer-is-valid or not. If we had that, @arichardson could define that AS200 does not have valid null pointers. If you do that right now the IR passes will at least deduce nonnull based on derferenceable.

[rsmith]

clang/lib/CodeGen/CGCall.cpp

2169

I think all of this is correct except that the frontend does know what the bit-pattern of the null pointer is in any particular language-level address space

Interesting. How do we get at that? Do we ask the middle-end to constant-fold addrspacecast i8* null to i8* addrspace(N) and see if we get a null back, or is there a better way?

In any case, this patch follows the same pattern we use for return values of reference type, parameters of reference type, and decayed array function parameters with static array bounds, all of which apply nonnull only in address space 0. If we want to use a different pattern, to consider whether LLVM's nonnull means "not a null pointer" rather than assuming that holds only in address space 0, we should make a holistic change for that throughout CGCall.cpp, rather than touching only the handling of the this pointer.

It'd also be interesting to consider what we want __attribute__((nonnull)) to mean in address spaces where the null pointer isn't the zero pointer: should it mean non-zero at the source level / non-null in LLVM IR, or should it mean non-null at the source level (which might be unrepresentable in LLVM IR, but static analysis etc. could still detect it)? We're currently inconsistent on this: static analysis, constant evaluation, and sanitizers treat the attribute as meaning non-null, but IR generation emits the nonnull IR attribute, treating it as meaning non-zero instead.

[rjmccall]

clang/lib/CodeGen/CGCall.cpp

2169

Interesting. How do we get at that? Do we ask the middle-end to constant-fold addrspacecast i8* null to i8* addrspace(N) and see if we get a null back, or is there a better way?

In fact, the middle-end has no ability to constant-fold addrspacecast, because LLVM's address-space support is a giant pile of hacks rather than exhibiting any cohesive design principles; or to put it another way, the lack of any centralized technical design authority in LLVM means that nobody has been able to enforce any rules about, say, targets being able to contribute knowledge about address spaces to the middle-end.

But in the frontend, we essentially have our own independent concept of address spaces, and the mapping from an AST address space to an IR address space can be non-obvious (and, in particular, non-injective), targets can perform arbitrary IR for address-space conversions instead of just using addrspacecast, and so on. This is necessary because we have to actually ship a functional compiler, whether with LLVM or despite it. And one of the places where it's necessary to work around LLVM is emitting a null pointer constant, because of course C requires a null pointer constant to be a constant expression, which in LLVM means we have to produce an llvm::Constant* for it, and we don't get to rely on assumptions like addrspacecast having appropriate semantics. So instead we just have ASTContext::getTargetNullPointerValue, which is ultimately fed by the TargetInfo, and we expect that to be consistent with the behavior of e.g. the address-space-conversion customization point.

I think __attribute__((nonnull)) clearly needs to be based on the language-level null pointer concept rather than having anything to do with a zero bit representation, and if that means we can't use it in IR, maybe we can improve IR.

[arichardson]

clang/lib/CodeGen/CGCall.cpp

2169

I think all of this is correct except that the frontend does know what the bit-pattern of the null pointer is in any particular language-level address space

Interesting. How do we get at that? Do we ask the middle-end to constant-fold addrspacecast i8* null to i8* addrspace(N) and see if we get a null back, or is there a better way?

We try rather hard to avoid any addrspacecasts in the IR and emit i8* addrspace(200) null for language-level NULL pointers.

A bit more context: In our compiler we support two compilation modes: so-called "pure-capability" where we use AS 200 for all pointers. This maps to using capability instructions in the backend (effectively 128-bit fat pointers with a hidden validity tag). We also support a hybrid compilation mode where we emit everything in AS0 (64-bit integer pointers) by default and only use AS200 for annotated language-level pointers.
In both of these address spaces a zero bit-pattern is NULL and not dereferenceable.
In the "pure-capability" mode our AS200 is effectively the default AS0. However, we can't use that since we need to use a different AS to select our instructions in the backend since we extend the MIPS,RISC-V and AArch64 backends and AS0 already maps to integer loads/stores/etc.

So the problem as I see it is that clang needs to be able to tell that a for a given LLVM IR address space llvm IR null (i.e. all zero bits) maps to C/C++ language-level NULL/nullptr?

However, I did not see anywhere in the langref that null is equivalent to "all-zero-bits", but I may have missed that part.
All I saw about null was:

Null pointer constants
The identifier ‘null’ is recognized as a null pointer constant and must be of pointer type.

So unless I missed something it seems like AS<N> could define null as being all-ones instead, and that could then map to the representation of language-level NULL on machines where it is not all-zeroes?

In our fork I added a local hack to replace the checks for adding the nonnull attribute if AS==0 with a canMarkAsNonNull helper that checks for AS==0||AS==200. Maybe there should be a helper function that checks whether language-level NULL maps to null in address space N, but that would probably require making null-pointer-is-valid a per-AS property?

[rjmccall]

clang/lib/CodeGen/CGCall.cpp

2169

LLVM's assumption that null (ConstantPointerNull) is a zero bit-pattern is reflected in places like isNullValue(), which (despite the name) actually determines whether the value is a zero bit-pattern, as you can see from how it operates on ConstantFP and so on.

But if your null pointers are actually the zero bit-pattern across all address spaces, I don't think that would cause you any problems.

We should be able to just replace this check with getContext().getTargetNullPointerValue(FI.arg_begin()->type) == 0.

[arichardson]

clang/lib/CodeGen/CGCall.cpp

2169

That sounds like a good solution, and we should probably also be explicit in the Langref that null is assumed to be a zero bit-pattern.

[brooksmoses]

So, I have bad news: This causes OpenJDK to segfault. The relevant code is here:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/memory/arena.cpp#L311

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?

[bkramer]

In D17993#2409401, @brooksmoses wrote:

So, I have bad news: This causes OpenJDK to segfault. The relevant code is here:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/memory/arena.cpp#L311

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?

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:
https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L842

Is the build system not setting this when using Clang?

[xbolva00]

In that code there is a comment:

These flags are required for GCC 6 builds as undefined behaviour in OpenJDK code

So you should really fix UB...

[brooksmoses]

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

clang/lib/CodeGen/CGCall.cpp

void CodeGenModule::ConstructAttributeList(
   // Attach attributes to inalloca argument.
   if (IRFunctionArgs.hasInallocaArg()) {
     llvm::AttrBuilder Attrs;
     Attrs.addAttribute(llvm::Attribute::InAlloca);
     ArgAttrs[IRFunctionArgs.getInallocaArgNo()] =
         llvm::AttributeSet::get(getLLVMContext(), Attrs);
   }
 
+  // Apply `nonnull` and `dereferencable(N)` to the `this` argument.
+  if (FI.isInstanceMethod() && !IRFunctionArgs.hasInallocaArg() &&
+      !FI.arg_begin()->type->isVoidPointerType()) {
+    auto IRArgs = IRFunctionArgs.getIRArgs(0);
+
+    assert(IRArgs.second == 1 && "Expected only a single `this` pointer.");
+
+    llvm::AttrBuilder Attrs;
+
+    if (!CodeGenOpts.NullPointerIsValid &&
+        getContext().getTargetAddressSpace(FI.arg_begin()->type) == 0) {
+      Attrs.addAttribute(llvm::Attribute::NonNull);
+    }
+
+    Attrs.addDereferenceableAttr(
+        getMinimumObjectSize(
+            FI.arg_begin()->type.castAs<PointerType>()->getPointeeType())
+            .getQuantity());
+
+    ArgAttrs[IRArgs.first] = llvm::AttributeSet::get(getLLVMContext(), Attrs);
+  }
+
   unsigned ArgNo = 0;
   for (CGFunctionInfo::const_arg_iterator I = FI.arg_begin(),
                                           E = FI.arg_end();
        I != E; ++I, ++ArgNo) {
     QualType ParamType = I->type;
     const ABIArgInfo &AI = I->info;
     llvm::AttrBuilder Attrs;
 

clang/test/CodeGenCXX/array-default-argument.cpp

 };
 
 void f();
 // CHECK-LABEL: define {{(dso_local )?}}void @_Z1gv()
 void g() {
   // CHECK: br label %[[LOOP:.*]]
 
   // [[LOOP]]:
-  // CHECK: {{call|invoke}} {{.*}} @_ZN1AC1Ev([[TEMPORARY:.*]])
+  // CHECK: {{call|invoke}} {{.*}} @_ZN1AC1Ev({{.*}} [[TEMPORARY:%.*]])
   // CHECK-EH:  unwind label %[[PARTIAL_ARRAY_LPAD:.*]]
-  // CHECK: {{call|invoke}} {{.*}} @_ZN1BC1E1A({{.*}}, [[TEMPORARY]])
+  // CHECK: {{call|invoke}} {{.*}} @_ZN1BC1E1A({{.*}} [[TEMPORARY]])
   // CHECK-EH:  unwind label %[[A_AND_PARTIAL_ARRAY_LPAD:.*]]
-  // CHECK: {{call|invoke}} {{.*}} @_ZN1AD1Ev([[TEMPORARY]])
+  // CHECK: {{call|invoke}} {{.*}} @_ZN1AD1Ev({{.*}} [[TEMPORARY]])
   // CHECK-EH:  unwind label %[[PARTIAL_ARRAY_LPAD]]
   // CHECK: getelementptr {{.*}}, i{{[0-9]*}} 1
   // CHECK: icmp eq
   // CHECK: br i1 {{.*}} label %[[LOOP]]
   B b[5];
 
   // CHECK: {{call|invoke}} void @_Z1fv()
   f();
 
   // CHECK-NOT: @_ZN1AD1Ev(
   // CHECK: {{call|invoke}} {{.*}} @_ZN1BD1Ev(
 }

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

// CHECK: %[[CHILD:.*]] = load %struct.ChildNoOverride
 
   child->left();
 // Only need to cast 'this' to Left*.
 // CHECK: %[[LEFT:.*]] = bitcast %struct.ChildNoOverride* %[[CHILD]] to %struct.Left*
 // CHECK: %[[VFPTR:.*]] = bitcast %struct.Left* %[[LEFT]] to void (%struct.Left*)***
 // CHECK: %[[VFTABLE:.*]] = load void (%struct.Left*)**, void (%struct.Left*)*** %[[VFPTR]]
 // CHECK: %[[VFUN:.*]] = getelementptr inbounds void (%struct.Left*)*, void (%struct.Left*)** %[[VFTABLE]], i64 0
 // CHECK: %[[VFUN_VALUE:.*]] = load void (%struct.Left*)*, void (%struct.Left*)** %[[VFUN]]
-// CHECK: call x86_thiscallcc void %[[VFUN_VALUE]](%struct.Left* %[[LEFT]])
+// CHECK: call x86_thiscallcc void %[[VFUN_VALUE]](%struct.Left* {{[^,]*}} %[[LEFT]])
 // CHECK: ret
 }
 
 void ChildOverride::left() {
 // CHECK-LABEL: define dso_local x86_thiscallcc void @"?left@ChildOverride@@UAEXXZ"
-// CHECK-SAME: (%struct.ChildOverride* %[[THIS:.*]])
+// CHECK-SAME: (%struct.ChildOverride* {{[^,]*}} %[[THIS:.*]])
 //
 // No need to adjust 'this' as the ChildOverride's layout begins with Left.
 // CHECK: %[[THIS_ADDR:.*]] = alloca %struct.ChildOverride*, align 4
 // CHECK: store %struct.ChildOverride* %[[THIS]], %struct.ChildOverride** %[[THIS_ADDR]], align 4
 
   foo(this);
 // CHECK: %[[THIS:.*]] = load %struct.ChildOverride*, %struct.ChildOverride** %[[THIS_ADDR]]
 // CHECK: %[[THIS_i8:.*]] = bitcast %struct.ChildOverride* %[[THIS]] to i8*
 // CHECK: call void @foo(i8* %[[THIS_i8]])
 // CHECK: ret
 }
 
 void call_left_override(ChildOverride *child) {
 // CHECK-LABEL: define dso_local void @"?call_left_override
 // CHECK: %[[CHILD:.*]] = load %struct.ChildOverride
 
   child->left();
 // CHECK: %[[VFPTR:.*]] = bitcast %struct.ChildOverride* %[[CHILD]] to void (%struct.ChildOverride*)***
 // CHECK: %[[VFTABLE:.*]] = load void (%struct.ChildOverride*)**, void (%struct.ChildOverride*)*** %[[VFPTR]]
 // CHECK: %[[VFUN:.*]] = getelementptr inbounds void (%struct.ChildOverride*)*, void (%struct.ChildOverride*)** %[[VFTABLE]], i64 0
 // CHECK: %[[VFUN_VALUE:.*]] = load void (%struct.ChildOverride*)*, void (%struct.ChildOverride*)** %[[VFUN]]
 //
-// CHECK: call x86_thiscallcc void %[[VFUN_VALUE]](%struct.ChildOverride* %[[CHILD]])
+// CHECK: call x86_thiscallcc void %[[VFUN_VALUE]](%struct.ChildOverride* {{[^,]*}} %[[CHILD]])
 // CHECK: ret
 }
 
 void call_right_no_override(ChildNoOverride *child) {
 // CHECK-LABEL: define dso_local void @"?call_right_no_override
 // CHECK: %[[CHILD:.*]] = load %struct.ChildNoOverride
 
   child->right();
 // When calling a right base's virtual method, one needs to adjust 'this' at
 // the caller site.
 //
 // CHECK: %[[CHILD_i8:.*]] = bitcast %struct.ChildNoOverride* %[[CHILD]] to i8*
 // CHECK: %[[RIGHT_i8:.*]] = getelementptr inbounds i8, i8* %[[CHILD_i8]], i32 4
 // CHECK: %[[RIGHT:.*]] = bitcast i8* %[[RIGHT_i8]] to %struct.Right*
 //
 // CHECK: %[[VFPTR:.*]] = bitcast %struct.Right* %[[RIGHT]] to void (%struct.Right*)***
 // CHECK: %[[VFTABLE:.*]] = load void (%struct.Right*)**, void (%struct.Right*)*** %[[VFPTR]]
 // CHECK: %[[VFUN:.*]] = getelementptr inbounds void (%struct.Right*)*, void (%struct.Right*)** %[[VFTABLE]], i64 0
 // CHECK: %[[VFUN_VALUE:.*]] = load void (%struct.Right*)*, void (%struct.Right*)** %[[VFUN]]
-// CHECK: call x86_thiscallcc void %[[VFUN_VALUE]](%struct.Right* %[[RIGHT]])
+// CHECK: call x86_thiscallcc void %[[VFUN_VALUE]](%struct.Right* {{[^,]*}} %[[RIGHT]])
 // CHECK: ret
 }
 
 void ChildOverride::right() {
 // CHECK-LABEL: define dso_local x86_thiscallcc void @"?right@ChildOverride@@UAEXXZ"(i8*
 //
 // ChildOverride::right gets 'this' cast to Right* in ECX (i.e. this+4) so we
 // need to adjust 'this' before use.

 //
 // CHECK: %[[CHILD_i8:.*]] = bitcast %struct.ChildOverride* %[[CHILD]] to i8*
 // CHECK: %[[VFPTR_i8:.*]] = getelementptr inbounds i8, i8* %[[CHILD_i8]], i32 4
 // CHECK: %[[VFPTR:.*]] = bitcast i8* %[[VFPTR_i8]] to void (i8*)***
 // CHECK: %[[VFTABLE:.*]] = load void (i8*)**, void (i8*)*** %[[VFPTR]]
 // CHECK: %[[VFUN:.*]] = getelementptr inbounds void (i8*)*, void (i8*)** %[[VFTABLE]], i64 0
 // CHECK: %[[VFUN_VALUE:.*]] = load void (i8*)*, void (i8*)** %[[VFUN]]
 //
 // CHECK: call x86_thiscallcc void %[[VFUN_VALUE]](i8* %[[RIGHT]])
 // CHECK: ret
 }
 
 struct GrandchildOverride : ChildOverride {
   virtual void right();
 };
 
 void GrandchildOverride::right() {

 
 struct AsymmetricChild : LeftWithNonVirtualDtor, Right {
   virtual ~AsymmetricChild();
 };
 
 void call_asymmetric_child_complete_dtor() {
   // CHECK-LABEL: define dso_local void @"?call_asymmetric_child_complete_dtor@@YAXXZ"
   AsymmetricChild obj;
-  // CHECK: call x86_thiscallcc %struct.AsymmetricChild* @"??0AsymmetricChild@@QAE@XZ"(%struct.AsymmetricChild* %[[OBJ:.*]])
+  // CHECK: call x86_thiscallcc %struct.AsymmetricChild* @"??0AsymmetricChild@@QAE@XZ"(%struct.AsymmetricChild* {{[^,]*}} %[[OBJ:.*]])
   // CHECK-NOT: getelementptr
-  // CHECK: call x86_thiscallcc void @"??1AsymmetricChild@@UAE@XZ"(%struct.AsymmetricChild* %[[OBJ]])
+  // CHECK: call x86_thiscallcc void @"??1AsymmetricChild@@UAE@XZ"(%struct.AsymmetricChild* {{[^,]*}} %[[OBJ]])
   // CHECK: ret
 }

clang/test/CodeGenCXX/this-nonnull.cpp

+// RUN: %clang_cc1 -S -emit-llvm -o - -triple=x86_64-linux-gnu %s | FileCheck %s -check-prefix=CHECK-YES
+// RUN: %clang_cc1 -S -emit-llvm -o - -fno-delete-null-pointer-checks -triple=x86_64-linux-gnu %s | FileCheck %s -check-prefix=CHECK-NO
+
+struct Struct {
+  int many;
+  int member;
+  int fields;
+  void ReturnsVoid();
+};
+
+void TestReturnsVoid(Struct &s) {
+  s.ReturnsVoid();
+
+  // CHECK-YES: call void @_ZN6Struct11ReturnsVoidEv(%struct.Struct* nonnull dereferenceable(12) %0)
+  // CHECK-NO: call void @_ZN6Struct11ReturnsVoidEv(%struct.Struct* dereferenceable(12) %0)
+}
+
+// CHECK-YES: declare void @_ZN6Struct11ReturnsVoidEv(%struct.Struct* nonnull dereferenceable(12))
+// CHECK-NO: declare void @_ZN6Struct11ReturnsVoidEv(%struct.Struct* dereferenceable(12))