Page MenuHomePhabricator

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

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

Details

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
CJ-Johnson added inline comments.
include/clang/Driver/Options.td
1157

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.Oct 12 2020, 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

Herald added a project: Restricted Project. Β· View Herald TranscriptOct 12 2020, 11:19 AM
arsenm added inline comments.Oct 12 2020, 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.Oct 12 2020, 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.Oct 12 2020, 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.Oct 12 2020, 11:29 AM
clang/lib/CodeGen/CGCall.cpp
2174 β†—(On Diff #297647)

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.Oct 12 2020, 11:53 AM
CJ-Johnson added inline comments.Oct 12 2020, 11:57 AM
clang/lib/CodeGen/CGCall.cpp
2174 β†—(On Diff #297647)

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.Oct 12 2020, 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 β†—(On Diff #297655)

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.Oct 12 2020, 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 β†—(On Diff #297647)

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 β†—(On Diff #297655)

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 β†—(On Diff #297655)

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.Oct 12 2020, 3:03 PM
jdoerfert added inline comments.Oct 12 2020, 4:56 PM
clang/lib/CodeGen/CGCall.cpp
2174 β†—(On Diff #297647)

(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.Oct 14 2020, 11:46 AM
clang/lib/CodeGen/CGCall.cpp
2165 β†—(On Diff #297655)

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 β†—(On Diff #297647)

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

CJ-Johnson marked 6 inline comments as done.Oct 31 2020, 9:30 AM
CJ-Johnson marked an inline comment as done.Oct 31 2020, 9:49 AM
CJ-Johnson added inline comments.
clang/test/CodeGenCXX/microsoft-abi-multiple-nonvirtual-inheritance.cpp
126 β†—(On Diff #297655)

Fixed! Added a check for isVoidPointerType() :)

CJ-Johnson marked an inline comment as done.

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

rsmith added a comment.Nov 1 2020, 6:24 PM

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
1–2 β†—(On Diff #302107)

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

CJ-Johnson updated this revision to Diff 302236.Nov 2 2020, 3:11 AM
CJ-Johnson marked an inline comment as done.

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 accepted this revision.Fri, Nov 6, 5:20 PM
rsmith added inline comments.
clang/test/CodeGenCXX/this-nonnull.cpp
1–2 β†—(On Diff #302236)

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

This revision is now accepted and ready to land.Fri, Nov 6, 5:20 PM
CJ-Johnson updated this revision to Diff 303648.Sat, Nov 7, 9:03 AM
CJ-Johnson marked an inline comment as done.
CJ-Johnson added inline comments.Sat, Nov 7, 9:05 AM
clang/test/CodeGenCXX/this-nonnull.cpp
1–2 β†—(On Diff #302236)

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

Please modify the commit subject and add a proper message.

CJ-Johnson retitled this revision from [CodeGen] Apply 'nonnull' to 'this' pointer arguments. to [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments..Mon, Nov 16, 3:35 PM
CJ-Johnson edited the summary of this revision. (Show Details)

Please modify the commit subject and add a proper message.

Thank you for the reminder! It slipped my mind.

Fixed :)

jdoerfert accepted this revision.Mon, Nov 16, 3:49 PM

LGTM, thx!

Herald added a project: Restricted Project. Β· View Herald TranscriptMon, Nov 16, 5:39 PM
arichardson added inline comments.
clang/lib/CodeGen/CGCall.cpp
2169 β†—(On Diff #305632)

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 added inline comments.Tue, Nov 17, 3:18 PM
clang/lib/CodeGen/CGCall.cpp
2169 β†—(On Diff #305632)

I also think NullPointerIsValid is sufficient.

rsmith added inline comments.Tue, Nov 17, 3:36 PM
clang/lib/CodeGen/CGCall.cpp
2169 β†—(On Diff #305632)

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 added inline comments.Tue, Nov 17, 3:44 PM
clang/lib/CodeGen/CGCall.cpp
2169 β†—(On Diff #305632)

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 added inline comments.Tue, Nov 17, 3:49 PM
clang/lib/CodeGen/CGCall.cpp
2169 β†—(On Diff #305632)

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 added inline comments.Tue, Nov 17, 4:29 PM
clang/lib/CodeGen/CGCall.cpp
2169 β†—(On Diff #305632)

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 added inline comments.Tue, Nov 17, 6:11 PM
clang/lib/CodeGen/CGCall.cpp
2169 β†—(On Diff #305632)

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 added inline comments.Wed, Nov 18, 4:12 AM
clang/lib/CodeGen/CGCall.cpp
2169 β†—(On Diff #305632)

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 added inline comments.Wed, Nov 18, 11:19 AM
clang/lib/CodeGen/CGCall.cpp
2169 β†—(On Diff #305632)

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 added inline comments.Thu, Nov 19, 3:32 AM
clang/lib/CodeGen/CGCall.cpp
2169 β†—(On Diff #305632)

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.

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?

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?

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

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