[llvm] 2f3b7d3 - [Inliner] Fix bug when propagating poison generating return attributes
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 4 11:08:42 PDT 2023
I'm not convinced that the reasoning in this patch is correct. I'm also
not sure I'm correct here, as since the last time I looked at our
nonnull semantics, we appear to have weakened them from immediate UB to
poison generating.
You give the example:
```
define nonnull ptr @foo() {
%p = call ptr @bar()
call void @use(ptr %p)
ret ptr %p
}
```
You're entirely correct that we can't propagate nonnull to the return
attributes on the @bar callsite blindly. You don't says this clearly in
the commit message, but the reason is that @use may throw an exception
if passed a null pointer, and thus the poison on return from @foo may
never be triggered.
Now what I'm not sure about is what happens if we know @use doesn't
throw, and must return. If nonnull was still immediate UB, then the
transform would be legal. Once you've proven UB must execute, it's
allowed to "time travel". Is poison allowed to time travel? I don't know.
However, you go on to give this example:
```
define nonnull ptr @foo() {
%p = call noundef ptr @bar()
ret ptr %p
}
```
If I gather correctly, this is basically an argument that poison can't
time travel. Even through we know a null result from @bar will produce
poison from @foo, you appear to be claiming that such a program is not
immediate UB, but instead deferred UB - if the poison is used.
Is that the semantics as you understand them? If so, then my concern is
invalid.
Philip
On 9/28/23 15:27, Noah Goldstein via llvm-commits wrote:
> Author: Noah Goldstein
> Date: 2023-09-28T17:27:42-05:00
> New Revision: 2f3b7d33f421b723728262a6978041d93f1f8c5c
>
> URL: https://github.com/llvm/llvm-project/commit/2f3b7d33f421b723728262a6978041d93f1f8c5c
> DIFF: https://github.com/llvm/llvm-project/commit/2f3b7d33f421b723728262a6978041d93f1f8c5c.diff
>
> LOG: [Inliner] Fix bug when propagating poison generating return attributes
>
> Poison generating return attributes can't be propagated the same as
> others, as they can change the behavior of other uses and/or create UB
> where it otherwise wouldn't have occurred.
>
> For example:
> ```
> define nonnull ptr @foo() {
> %p = call ptr @bar()
> call void @use(ptr %p)
> ret ptr %p
> }
> ```
>
> If we inline `@foo` and propagate `nonnull` to `@bar`, it could change
> the behavior of `@use` as instead of taking `null`, `@use` will
> now be passed `poison`.
>
> This can be even worth in a case like:
> ```
> define nonnull ptr @foo() {
> %p = call noundef ptr @bar()
> ret ptr %p
> }
> ```
>
> Where propagating `nonnull` to `@bar` will cause UB on `null` return
> of `@bar` (`noundef` + `poison`) where it previously wouldn't
> have occurred.
>
> To fix this, we only propagate poison generating return attributes if
> either 1) The only use of the callsite to propagate too is return and
> the callsite to propagate too doesn't have `noundef`. Or 2) the
> callsite to be be inlined has `noundef`.
>
> The former case ensures no new UB or `poison` values will be
> added. The latter is UB anyways if the value is `poison` so we can go
> ahead without worrying about behavior changes.
>
> Added:
>
>
> Modified:
> llvm/lib/Transforms/Utils/InlineFunction.cpp
> llvm/test/Transforms/Inline/ret_attr_align_and_noundef.ll
>
> Removed:
>
>
>
> ################################################################################
> diff --git a/llvm/lib/Transforms/Utils/InlineFunction.cpp b/llvm/lib/Transforms/Utils/InlineFunction.cpp
> index 56bc90a7c935292..548f949277952ca 100644
> --- a/llvm/lib/Transforms/Utils/InlineFunction.cpp
> +++ b/llvm/lib/Transforms/Utils/InlineFunction.cpp
> @@ -1344,25 +1344,36 @@ static bool MayContainThrowingOrExitingCallAfterCB(CallBase *Begin,
> ++BeginIt, End->getIterator(), InlinerAttributeWindow + 1);
> }
>
> -static AttrBuilder IdentifyValidAttributes(CallBase &CB) {
> +// Only allow these white listed attributes to be propagated back to the
> +// callee. This is because other attributes may only be valid on the call
> +// itself, i.e. attributes such as signext and zeroext.
> +
> +// Attributes that are always okay to propagate as if they are violated its
> +// immediate UB.
> +static AttrBuilder IdentifyValidUBGeneratingAttributes(CallBase &CB) {
> AttrBuilder Valid(CB.getContext());
> - // Only allow these white listed attributes to be propagated back to the
> - // callee. This is because other attributes may only be valid on the call
> - // itself, i.e. attributes such as signext and zeroext.
> if (auto DerefBytes = CB.getRetDereferenceableBytes())
> Valid.addDereferenceableAttr(DerefBytes);
> if (auto DerefOrNullBytes = CB.getRetDereferenceableOrNullBytes())
> Valid.addDereferenceableOrNullAttr(DerefOrNullBytes);
> if (CB.hasRetAttr(Attribute::NoAlias))
> Valid.addAttribute(Attribute::NoAlias);
> + return Valid;
> +}
> +
> +// Attributes that need additional checks as propagating them may change
> +// behavior or cause new UB.
> +static AttrBuilder IdentifyValidPoisonGeneratingAttributes(CallBase &CB) {
> + AttrBuilder Valid(CB.getContext());
> if (CB.hasRetAttr(Attribute::NonNull))
> Valid.addAttribute(Attribute::NonNull);
> return Valid;
> }
>
> static void AddReturnAttributes(CallBase &CB, ValueToValueMapTy &VMap) {
> - AttrBuilder Valid = IdentifyValidAttributes(CB);
> - if (!Valid.hasAttributes())
> + AttrBuilder ValidUB = IdentifyValidUBGeneratingAttributes(CB);
> + AttrBuilder ValidPG = IdentifyValidPoisonGeneratingAttributes(CB);
> + if (!ValidUB.hasAttributes() && !ValidPG.hasAttributes())
> return;
> auto *CalledFunction = CB.getCalledFunction();
> auto &Context = CalledFunction->getContext();
> @@ -1406,7 +1417,55 @@ static void AddReturnAttributes(CallBase &CB, ValueToValueMapTy &VMap) {
> // existing attribute value (i.e. attributes such as dereferenceable,
> // dereferenceable_or_null etc). See AttrBuilder::merge for more details.
> AttributeList AL = NewRetVal->getAttributes();
> - AttributeList NewAL = AL.addRetAttributes(Context, Valid);
> + AttributeList NewAL = AL.addRetAttributes(Context, ValidUB);
> + // Attributes that may generate poison returns are a bit tricky. If we
> + // propagate them, other uses of the callsite might have their behavior
> + // change or cause UB (if they have noundef) b.c of the new potential
> + // poison.
> + // Take the following three cases:
> + //
> + // 1)
> + // define nonnull ptr @foo() {
> + // %p = call ptr @bar()
> + // call void @use(ptr %p) willreturn nounwind
> + // ret ptr %p
> + // }
> + //
> + // 2)
> + // define noundef nonnull ptr @foo() {
> + // %p = call ptr @bar()
> + // call void @use(ptr %p) willreturn nounwind
> + // ret ptr %p
> + // }
> + //
> + // 3)
> + // define nonnull ptr @foo() {
> + // %p = call noundef ptr @bar()
> + // ret ptr %p
> + // }
> + //
> + // In case 1, we can't propagate nonnull because poison value in @use may
> + // change behavior or trigger UB.
> + // In case 2, we don't need to be concerned about propagating nonnull, as
> + // any new poison at @use will trigger UB anyways.
> + // In case 3, we can never propagate nonnull because it may create UB due to
> + // the noundef on @bar.
> + if (ValidPG.hasAttributes()) {
> + // Three checks.
> + // If the callsite has `noundef`, then a poison due to violating the
> + // return attribute will create UB anyways so we can always propagate.
> + // Otherwise, if the return value (callee to be inlined) has `noundef`, we
> + // can't propagate as a new poison return will cause UB.
> + // Finally, check if the return value has no uses whose behavior may
> + // change/may cause UB if we potentially return poison. At the moment this
> + // is implemented overly conservatively with a single-use check.
> + // TODO: Update the single-use check to iterate through uses and only bail
> + // if we have a potentially dangerous use.
> +
> + if (CB.hasRetAttr(Attribute::NoUndef) ||
> + (RetVal->hasOneUse() && !RetVal->hasRetAttr(Attribute::NoUndef)))
> + NewAL = NewAL.addRetAttributes(Context, ValidPG);
> + }
> NewRetVal->setAttributes(NewAL);
> }
> }
>
> diff --git a/llvm/test/Transforms/Inline/ret_attr_align_and_noundef.ll b/llvm/test/Transforms/Inline/ret_attr_align_and_noundef.ll
> index f7dccc75ba4e029..ddb94ef3fe4e070 100644
> --- a/llvm/test/Transforms/Inline/ret_attr_align_and_noundef.ll
> +++ b/llvm/test/Transforms/Inline/ret_attr_align_and_noundef.ll
> @@ -206,7 +206,7 @@ define ptr @callee9() {
>
> define ptr @caller9_fail_creates_ub() {
> ; CHECK-LABEL: define ptr @caller9_fail_creates_ub() {
> -; CHECK-NEXT: [[R_I:%.*]] = call noundef nonnull ptr @foo()
> +; CHECK-NEXT: [[R_I:%.*]] = call noundef ptr @foo()
> ; CHECK-NEXT: call void @use.ptr(ptr [[R_I]])
> ; CHECK-NEXT: ret ptr [[R_I]]
> ;
> @@ -239,7 +239,7 @@ define ptr @callee10() {
>
> define ptr @caller10_fail_maybe_poison() {
> ; CHECK-LABEL: define ptr @caller10_fail_maybe_poison() {
> -; CHECK-NEXT: [[R_I:%.*]] = call nonnull ptr @foo()
> +; CHECK-NEXT: [[R_I:%.*]] = call ptr @foo()
> ; CHECK-NEXT: call void @use.ptr(ptr [[R_I]])
> ; CHECK-NEXT: ret ptr [[R_I]]
> ;
> @@ -259,7 +259,7 @@ define ptr @caller10_okay_will_be_ub() {
>
> define noundef ptr @caller10_okay_will_be_ub_todo() {
> ; CHECK-LABEL: define noundef ptr @caller10_okay_will_be_ub_todo() {
> -; CHECK-NEXT: [[R_I:%.*]] = call nonnull ptr @foo()
> +; CHECK-NEXT: [[R_I:%.*]] = call ptr @foo()
> ; CHECK-NEXT: call void @use.ptr(ptr [[R_I]])
> ; CHECK-NEXT: ret ptr [[R_I]]
> ;
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list