Page MenuHomePhabricator

llvm: Add support for "-fno-delete-null-pointer-checks"
ClosedPublic

Authored by manojgupta on Jun 7 2018, 11:17 AM.

Details

Summary

Support for this option is needed for building Linux kernel.
This is a very frequently requested feature by kernel developers.

More details : https://lkml.org/lkml/2018/4/4/601

GCC option description for -fdelete-null-pointer-checks:
This Assume that programs cannot safely dereference null pointers,
and that no code or data element resides at address zero.

-fno-delete-null-pointer-checks is the inverse of this implying that
null pointer dereferencing is not undefined.

This feature is implemented in LLVM IR in this CL as the function attribute
"null-pointer-is-valid"="true" in IR (Under review at D47894).
The CL updates several passes that assumed null pointer dereferencing is
undefined to not optimize when the "null-pointer-is-valid"="true"
attribute is present.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Updated some passes and added more test cases.

Missing LangRef changes.

Eli, Is this the place you are referring to?
https://llvm.org/docs/LangRef.html#attribute-groups

It does not cover much about the internal IR attributes, though I'll be happy to add "null-pointer-is-valid"="true" to the examples here.

Tests look good, at first glance.

For LangRef, I was referring to https://llvm.org/docs/LangRef.html#function-attributes, yes. (There might be some gaps in the documentation, but ideally that should cover all the function attributes recognized by LLVM transforms and code generation.)

It looks like you still didn't look at all the files I mentioned earlier?

include/llvm/Analysis/Utils/Local.h
93

Not sure I like the default argument here.

lib/Transforms/IPO/GlobalOpt.cpp
642

This doesn't handle address spaces correctly... at least, not at first glance.

Tests look good, at first glance.

For LangRef, I was referring to https://llvm.org/docs/LangRef.html#function-attributes, yes. (There might be some gaps in the documentation, but ideally that should cover all the function attributes recognized by LLVM transforms and code generation.)

Thanks, Will add it there.

It looks like you still didn't look at all the files I mentioned earlier?

I went through those files but many of these addressSpace() == 0 (and != 0) checks are not for null pointers but for disabling/skipping some memory optimizations. And it is not clear to me if moving all of those checks to the new NullPointerIsDefined function makes sense there.

lib/Transforms/IPO/GlobalOpt.cpp
1938

This pass already skips non-zero address space values. I didn't want to bail out here since all of the accesses to a GV might be in functions without null-pointer-is-valid attribute.

efriedma added inline comments.Jun 13 2018, 12:12 PM
lib/Transforms/IPO/GlobalOpt.cpp
1938

Fine; please add a comment to AllUsesOfValueWillTrapIfNull.

Added more test cases, updated LangRef, added a check for address space 0
in GlobalOpt.

I went through those files but many of these addressSpace() == 0 (and != 0) checks are not for null pointers but for disabling/skipping some memory optimizations. And it is not clear to me if moving all of those checks to the new NullPointerIsDefined function makes sense there.

We should probably add a section to LangRef on address spaces, and what transforms are specifically allowed only in address space 0.

I'm sure we need to handle null-pointer-is-valid in CallSite.h, at least.

docs/LangRef.rst
1467

Stating the ways to spell null pointer in C isn't helpful; this is the LLVM IR reference. A null pointer in IR is just "i8* null". (And the C/C++ notion of a "null pointer" can actually be translated to something which is not i8* null in some situations.)

Added checks to CallSite.h and LoopAccessAnalysis.cpp.
Added another test case for atomic loads.
Moved The NullPointerIsDefined function to Function.h.

Update LangRef.

xbolva00 added inline comments.Jun 18 2018, 1:00 PM
include/llvm/IR/Function.h
788 ↗(On Diff #151772)

Confusing..

nullPointerIsDefined or NullPointerIsDefined?

I went through those files but many of these addressSpace() == 0 (and != 0) checks are not for null pointers but for disabling/skipping some memory optimizations. And it is not clear to me if moving all of those checks to the new NullPointerIsDefined function makes sense there.

We should probably add a section to LangRef on address spaces, and what transforms are specifically allowed only in address space 0.

I'm sure we need to handle null-pointer-is-valid in CallSite.h, at least.

Thanks! I updated CallSite.h and also LoopAccessAnalysis. Had to find another home for NullPointerIsDefined function so moved it to Function.h.
I am in split mind about adding theses checks to BasicAliasAnalysis so need your opinion.

manojgupta added inline comments.Jun 18 2018, 1:03 PM
include/llvm/IR/Function.h
788 ↗(On Diff #151772)

This is part of Function Object so used lowercase to match the current code.

Added a comment in GlobalOpt.cpp

@efriedma I think I have addressed the changes you requested, can you ptal?

Please fix lib/Analysis/BasicAliasAnalysis.cpp to allow null pointers to alias other objects. Please fix lib/Analysis/LoopAccessAnalysis.cpp to allow a loop to dereference null. Please fix isGEPKnownNonNull in lib/Analysis/ValueTracking.cpp to allow objects located at null. Please fix ConstantFoldScalarCall in lib/Analysis/ConstantFolding.cpp to allow objects located at null. Please fix Argument::hasNonNullAttr in lib/IR/Function.cpp to allow objects at null.

There are probably other places which need to be fixed, but there's no good way to find them all; we'll just have to stumble over them later.

LangRef needs some work to clarify exactly what properties we assume about address-space zero besides the special properties of null, but I should probably write that myself.

docs/LangRef.rst
1465

Please specifically say "in address-space 0".

1468

undefined isn't a keyword.

Drive by comments

You also need to change the inliner to not inline null-pointer-is-dereferenceable into functions that don't have that attribute.

docs/LangRef.rst
1464

This should be called null-pointer-is-dereferenceable or something like that.

1466

Might be worth clarifying if this is just loads or both loads and stores.

1468

What about functions called from this function?

lib/Transforms/Utils/Local.cpp
1813–1815

I don't think we can fold calls to undef either -- the only reason why we can normally do this is because Callee could be null.

Made requested changes and added more tests.

manojgupta marked 4 inline comments as done.Jun 26 2018, 2:19 PM

Drive by comments

You also need to change the inliner to not inline null-pointer-is-dereferenceable into functions that don't have that attribute.

Done.
Note that, I am expecting that functions with alwaysinline attribute should still get inlined. If that happens and caller does not have this attribute, then optimizer is free to remove the checks.

Please fix lib/Analysis/BasicAliasAnalysis.cpp to allow null pointers to alias other objects. Please fix lib/Analysis/LoopAccessAnalysis.cpp to allow a loop to dereference null. Please fix isGEPKnownNonNull in lib/Analysis/ValueTracking.cpp to allow objects located at null. Please fix ConstantFoldScalarCall in lib/Analysis/ConstantFolding.cpp to allow objects located at null. Please fix Argument::hasNonNullAttr in lib/IR/Function.cpp to allow objects at null.

Done.

docs/LangRef.rst
1464

I actually had a typo here. The IR is using "null-pointer-is-valid" so changed to the same name.

1468

If the functions are not inlined, it shouldn't normally matter.
For functions that get inlined, I believe caller convention should be kept i.e. not propagating this attribute from callee to caller when inlining.

lib/IR/ConstantFold.cpp
1504 ↗(On Diff #152954)

There does not seem to be a way to find the Function and thereby the attribute here. Any suggestions?

Fixed typo in inlining unit test.

@efriedma : can you take a look again?
Thanks!

efriedma added inline comments.Jun 29 2018, 2:25 PM
lib/IR/ConstantFold.cpp
1504 ↗(On Diff #152954)

That's challenging... constants are, in general, outside any function, so we don't have any context for this. (More generally, I think ConstantExprs are a terrible idea, but we're basically stuck with them, at least for now.)

I guess in this particular case, we could just ignore the attribute and still get reasonable behavior; even if there's memory mapped at address 0, it probably isn't a GlobalValue. But please add a note here, and in LangRef.

lib/Transforms/IPO/GlobalOpt.cpp
1599

GV->getType() is the wrong type.

Fix an issue pointed by efriedma. All tests still pass.

manojgupta marked an inline comment as done.Jun 29 2018, 4:28 PM
manojgupta added inline comments.
lib/Transforms/IPO/GlobalOpt.cpp
1599

Done, Thanks for catching this.

manojgupta marked an inline comment as done.

Updated LangRef and added comment in ConstantFolding about inability
to query the attribute.

Looking good. I think I want to look at it one more time when I'm fresh on Monday, and give a bit of time for anyone else to look at it.

docs/LangRef.rst
1469

global variable, not "GlobalVariable" (that's what it's called in other places in the reference).

Replace GlobalVariable by global variable.

Rebase to master + handle constant folding of intrinsic strip_invariant_group.

Swooping in on this admittedly quite late: should the objectsize bits in MemoryBuiltins.cpp be made aware of this?

In particular, the ObjectSizeOffsetVisitor will happily hand you "0 bytes are accessible here" if you hand it null (...though not if you ask it via __builtin_object_size, since that has a gcc-compat hack around NULL, as well). We should probably make the NullIsUnknownSize field we pass into that factor in whether the current Function has this new null-is-ok attribute on it.

...Since it's possible for us to just hand it a ConstantPointerNull without a Function attached, we probably need to plumb this in from callers of llvm::getObjectSize/llvm::lowerObjectSizeCall/...

Swooping in on this admittedly quite late: should the objectsize bits in MemoryBuiltins.cpp be made aware of this?

In particular, the ObjectSizeOffsetVisitor will happily hand you "0 bytes are accessible here" if you hand it null (...though not if you ask it via __builtin_object_size, since that has a gcc-compat hack around NULL, as well). We should probably make the NullIsUnknownSize field we pass into that factor in whether the current Function has this new null-is-ok attribute on it.

...Since it's possible for us to just hand it a ConstantPointerNull without a Function attached, we probably need to plumb this in from callers of llvm::getObjectSize/llvm::lowerObjectSizeCall/...

I don't know much about __builtin_object_size but I think GCC compatibility is the key here. Does GCC returns different results for __builtin_object_size when "-fno-delete-null-pointer-checks" is specified?

I don't know much about __builtin_object_size but I think GCC compatibility is the key here. Does GCC returns different results for __builtin_object_size when "-fno-delete-null-pointer-checks" is specified?

__builtin_object_size lowers to llvm.objectsize with nullunknown = true, so we essentially treat it like an object anyway; it's not affected. The issue is for other users of llvm::getObjectSize (like alias analysis)

What Eli said :)

This can probably be fixed by setting NullIsUnknownSize appropriately in callers of llvm::getObjectSize.

(We'll likely also want to ensure that the objectsize sanitizer in clang sets the nullunknown bit when it lowers to calls of @llvm.objectsize if clang's been handed this flag, but that's a different patch)

manojgupta updated this revision to Diff 153985.Jul 3 2018, 2:47 PM

Handle getObjectSize by passing the ObjectSizeOpts in callers.

Adding George explicitly as a reviewer as I think this is now waiting on him.

Ah, sorry. I thought I hit submit, but apparently I didn't. :)

Objectsize-related changes look good to me, modulo a few tiny nits. Thanks!

lib/Analysis/InstructionSimplify.cpp
2125 ↗(On Diff #153985)

s/dyn_cast/cast/, please

(Or

if (auto *Foo = dyn_cast<AllocaInst>(LHS))
  if (isa<AllocaInst>(RHS) || isa<GlobalVariable>(RHS)) {
    // ...

; I'm impartial.)

lib/Transforms/Scalar/DeadStoreElimination.cpp
351 ↗(On Diff #153985)

const Function *F?

718 ↗(On Diff #153985)

const Function *F?

manojgupta updated this revision to Diff 154506.Jul 7 2018, 5:06 PM

Made suggested changes.

manojgupta marked 3 inline comments as done.Jul 7 2018, 5:08 PM
george.burgess.iv accepted this revision.Jul 7 2018, 7:36 PM

Nit fixes LGTM. Please wait for an official stamp from Eli before committing, since I can’t tell if he got the second look he wanted. (I’m inclined to believe he did, but don’t want to jump the gun)

Thanks for the patch!

This revision is now accepted and ready to land.Jul 7 2018, 7:36 PM
efriedma accepted this revision.Jul 9 2018, 2:12 PM

LGTM

This revision was automatically updated to reflect the committed changes.

Thanks everyone, specially Eli for the reviews.

Note that, I am expecting that functions with alwaysinline attribute should still get inlined. If that happens and caller does not have this attribute, then optimizer is free to remove the checks.

Not that I actually hit that, but it was questionable when reading the code - why that way? From LangRef: alwaysinline "indicates that the inliner should attempt to inline this function into callers whenever possible, ignoring any active inlining size threshold for this caller." So there is some freedom by not doing the inlining, even in the presence of the attribute.

Wouldn't it be better not to inline or to add the attribute to the caller?

Note that, I am expecting that functions with alwaysinline attribute should still get inlined. If that happens and caller does not have this attribute, then optimizer is free to remove the checks.

Not that I actually hit that, but it was questionable when reading the code - why that way? From LangRef: alwaysinline "indicates that the inliner should attempt to inline this function into callers whenever possible, ignoring any active inlining size threshold for this caller." So there is some freedom by not doing the inlining, even in the presence of the attribute.

Wouldn't it be better not to inline or to add the attribute to the caller?

Thanks for the comments.

Generally clang should add the attribute to all the functions if "-fno-delete-null-pointer-checks" is used so the mismatch should not happen in practice.

As per my understanding, If the "null-pointer-is-valid" is missing on a caller (but present on callee), this probably would happen in LTO where caller was intentionally compiled without "-fno-delete-null-pointer-checks". So adding the attribute to caller is probably not desired.
It was not clear to me if "-fno-delete-null-pointer-checks" mismatch should override always_inline but my inclination was to honor the user/system specified attribute over the compiler option.

I would be happy to change this logic if there is a consensus on how inlining should be handled in case of "null-pointer-isvalid" attribute mismatches.

Note that, I am expecting that functions with alwaysinline attribute should still get inlined. If that happens and caller does not have this attribute, then optimizer is free to remove the checks.

Not that I actually hit that, but it was questionable when reading the code - why that way? From LangRef: alwaysinline "indicates that the inliner should attempt to inline this function into callers whenever possible, ignoring any active inlining size threshold for this caller." So there is some freedom by not doing the inlining, even in the presence of the attribute.

Wouldn't it be better not to inline or to add the attribute to the caller?

Thanks for the comments.

Generally clang should add the attribute to all the functions if "-fno-delete-null-pointer-checks" is used so the mismatch should not happen in practice.

Random uneducated thought - how does it work with multiple translation units, where not all of them have/don't have that flag set?

As per my understanding, If the "null-pointer-is-valid" is missing on a caller (but present on callee), this probably would happen in LTO where caller was intentionally compiled without "-fno-delete-null-pointer-checks". So adding the attribute to caller is probably not desired.
It was not clear to me if "-fno-delete-null-pointer-checks" mismatch should override always_inline but my inclination was to honor the user/system specified attribute over the compiler option.

I would be happy to change this logic if there is a consensus on how inlining should be handled in case of "null-pointer-isvalid" attribute mismatches.

to change this logic if there is a consensus on how inlining should be handled in case of "null-pointer-isvalid" attribute mismatches.

I don't (and probably can't :) insist on changing the logic, but we should probably update the LangRef either in the entry for alwaysinline or in the entry of "null-pointer-isvalid". Otherwise, current transformation, IMO, breaks our own LangRef. Or do I miss something in its wording?

Random uneducated thought - how does it work with multiple translation units, where not all of them have/don't have that flag set?

Currently the logic disables inlining of functions with "null-pointer-is-valid" into functions that does not have this attribute.
However, if the callee has always_inline, inlining would still happen.

Here is an example of what GCC does in case of mismatched "fno-delete-null-pointer-checks".
https://godbolt.org/g/78Z2S4

There are 4 cases:
Case 1 : Attribute is on caller
Case 2 : Attribute is on callee
Case 3 : Attribute is on caller, callee has always_inline
Case 4 : Attribute is on callee, callee has always_inline

Inlining happens in all cases except (2). I think clang behavior matches GCC here.

Not sure of Langref changes, @efriedma What do you suggest?

If the user forces inlining with always_inline, we should probably be conservative and propagate the attribute. If we do that, I don't think we need to say anything about it in LangRef.

theraven removed a subscriber: theraven.Jul 29 2018, 2:20 AM