[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Julian Lettner via Phabricator via cfe-commits]

yln abandoned this revision.
yln added a comment.

Created new revision for this change: https://reviews.llvm.org/D57278


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56624/new/

https://reviews.llvm.org/D56624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

In D56624#1370607 , @yln wrote:

> In D56624#1370579 , @eugenis wrote:
>
> > Maybe the frontend should insert __asan_handle_noreturn whenever ASan is 
> > enabled, and then ASan would not care about the attribute? I'd like to 
> > avoid having this logic in two places.
>
>
> +1 for this. @vsk Can you sign off on this design?


Sounds good to me.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56624/new/

https://reviews.llvm.org/D56624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

In D56624#1370579 , @eugenis wrote:

> Maybe the frontend should insert __asan_handle_noreturn whenever ASan is 
> enabled, and then ASan would not care about the attribute? I'd like to avoid 
> having this logic in two places.


+1 for this. @vsk Can you sign off on this design?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56624/new/

https://reviews.llvm.org/D56624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

Maybe the frontend should insert __asan_handle_noreturn whenever ASan is 
enabled, and then ASan would not care about the attribute? I'd like to avoid 
having this logic in two places.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56624/new/

https://reviews.llvm.org/D56624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

Seems as if we reached consensus! :)  I will change the revision to use an 
intrinsic.

Before I start doing that, just one more quick idea:
Would it work if UBsan directly inserts calls to `__asan_handle_no_return` (of 
course only when ASan is requested). Similar to how it inserts calls to it's 
own runtime functions (e.g., `__ubsan_handle_builtin_unreachable`).
If we strive for the "simplest" solution... but maybe I am missing something in 
this is too simple?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56624/new/

https://reviews.llvm.org/D56624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

In D56624#1370280 , @eugenis wrote:

> > Wouldn’t it be preferable to unpoison the stack inside of maybe_longjmp, 
> > once the opaque condition can be checked?
>
> Sure, but that's not always possible. That's why we have interceptors.


Fair enough!

>>> One possible optimization that I can think of is splitting code after the 
>>> call into a separate basic block and marking it as cold.
>>>  Admittedly, that's unlikely to have big impact in practice. I'd guess that 
>>> [[expect_noreturn]] calls are typically not very hot in the first place.
>> 
>> The cold attribute is already used for this kind of splitting/reordering. I 
>> don't yet see how expect_noreturn creates new opportunities for the 
>> optimizer.
> 
> Strictly speaking, cold attribute on a function means that it is rarely 
> called. It does not say anything about the code after the call being colder 
> than the code before the call (within the same BB), which makes splitting the 
> BB pointless.

That's true, but it's safe to assume that code which dominates the cold call 
(or is post-dominated by it) is at least as cold as the call.

> Anyway, I agree that the arguments [[expect_noreturn]] are not that strong 
> and perhaps don't make the bar for the addition of a new IR attribute.
>  Should we go back to the intrinsic idea?

Sgtm, I think that'd be the simplest solution (something like inserting 
llvm.asan.stack_unpoison() where needed).


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56624/new/

https://reviews.llvm.org/D56624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

> Wouldn’t it be preferable to unpoison the stack inside of maybe_longjmp, once 
> the opaque condition can be checked?

Sure, but that's not always possible. That's why we have interceptors.

>> One possible optimization that I can think of is splitting code after the 
>> call into a separate basic block and marking it as cold.
>>  Admittedly, that's unlikely to have big impact in practice. I'd guess that 
>> [[expect_noreturn]] calls are typically not very hot in the first place.
> 
> The cold attribute is already used for this kind of splitting/reordering. I 
> don't yet see how expect_noreturn creates new opportunities for the optimizer.

Strictly speaking, cold attribute on a function means that it is rarely called. 
It does not say anything about the code after the call being colder than the 
code before the call (within the same BB), which makes splitting the BB 
pointless.

Anyway, I agree that the arguments [[expect_noreturn]] are not that strong and 
perhaps don't make the bar for the addition of a new IR attribute.
Should we go back to the intrinsic idea?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56624/new/

https://reviews.llvm.org/D56624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

In D56624#1370243 , @eugenis wrote:

> > Because "expect_noreturn" calls are allowed to return, the compiler must 
> > behave as they could. In particular, this means that unpoisoning the stack 
> > before expect_noreturn calls (given the current semantics) is premature.
>
> I don't think that's true. A hypothetical function
>
>   maybe_longjmp(jmp_buf env)
>
> that checks an opaque condition needs stack unpoisoning before the call, in 
> the absense of a better solution.


Wouldn’t it be preferable to unpoison the stack inside of maybe_longjmp, once 
the opaque condition can be checked? Even if not, a narrower sanitizer_noreturn 
attribute is still perfectly fine, here.

> One possible optimization that I can think of is splitting code after the 
> call into a separate basic block and marking it as cold.
>  Admittedly, that's unlikely to have big impact in practice. I'd guess that 
> [[expect_noreturn]] calls are typically not very hot in the first place.

The cold attribute is already used for this kind of splitting/reordering. I 
don't yet see how expect_noreturn creates new opportunities for the optimizer.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56624/new/

https://reviews.llvm.org/D56624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

> Because "expect_noreturn" calls are allowed to return, the compiler must 
> behave as they could. In particular, this means that unpoisoning the stack 
> before expect_noreturn calls (given the current semantics) is premature.

I don't think that's true. A hypothetical function

  maybe_longjmp(jmp_buf env)

that checks an opaque condition needs stack unpoisoning before the call, in the 
absense of a better solution.

One possible optimization that I can think of is splitting code after the call 
into a separate basic block and marking it as cold.
Admittedly, that's unlikely to have big impact in practice. I'd guess that 
[[expect_noreturn]] calls are typically not very hot in the first place.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56624/new/

https://reviews.llvm.org/D56624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

In D56624#1369940 , @yln wrote:

> Note that all of this currently only matters when compiling with 
> `-fsanitize=unreachable`. The following discussion is within the context of 
> the current implementation: UBSan removes the `noreturn` so it can instrument 
> `unreachable` without the added instrumentation being optimized away. Maybe 
> we should take a step back and ask if that is the right approach at all?
>
> In D56624#1369795 , @vsk wrote:
>
> > Because "expect_noreturn" calls are allowed to return, the compiler must 
> > behave as they could. In particular, this means that unpoisoning the stack 
> > before expect_noreturn calls (given the current semantics) is premature.
> >
> > Put another way, a frontend author may (understandably, but mistakenly!) 
> > attach expect_noreturn to calls which they expect to be cold.
>
>
> I think about this differently. Yes, most noreturn functions are also cold, 
> e.g., `abort`, but not necessarily, e.g., calls to `longjmp` do not 
> necessarily have to be. Why would it be okay to attach expect_noreturn 
> instead of cold?


It would be okay by definition, because it would be allowed by the proposed IR 
semantics.

> Why do we think that this is an easy-to-make mistake?

I don't think that's the right question. Rather, we should ask: why is it 
acceptable to define semantics in a way that makes the mistake possible?

My thinking on this is: it's not acceptable, because a narrower change (say, 
introducing a sanitizer_noreturn attribute) would address the issue without as 
much potential for abuse.

> Can we agree on the following?
>  "It is orthogonal on the language level, but seems to be redundant in terms 
> of the optimizer. Since LLVM IR's main purpose it support the optimizer, this 
> is a strong argument against the general purpose attribute."

I'm making a more neutral point: that expect_noreturn conflates different 
concerns -- optimization and sanitizer correctness. I'm not making a claim 
about what the main purpose of IR is.

>> That would regress ASan coverage.
> 
> You talk specifically about cases of misuses of the attribute, right?
>  In the context of the current issue with UBSan the possibility for false 
> negative is not too much of a regression: it only occurs when UBSan is going 
> to diagnose an "unreachable error" anyways.
> 
> So the main point is whether or not to use a "general purpose" attribute or a 
> "narrow purpose" attribute/intrinsic. My understanding is that you list the 
> following points as arguments against general purpose. Is my understanding 
> accurate?
> 
> 1. Potential misuse can regress ASan coverage
> 2. Complicates optimizer
> 
>   Narrow purpose: No potential misuses, and optimizer can simply ignore it.

Yes, I think this is a fair summary, thanks :).

> Initially I proposed a narrow purpose attribute, but while iterating on this 
> revision changed it to be general purpose. @eugenis 
>  Now, I have a slight preference for general purpose: I don't think 1. is a 
> big issue (but then again, I have no experience here),

Changes to the IR semantics have hard-to-predict ripple effects on many, many 
other projects. It pays to be conservative in this area.

>   and 2. it is always correct for the optimizer to continue ignoring the 
> attribute (current status).
> 
> Actually, 2. also encompasses the potential upside: a more complicated 
> optimizer that takes advantage of the attribute to do additional 
> optimizations.

I'm having a hard time thinking of any optimizations based on expect_noreturn 
which aren't already enabled by the cold attribute. What do you have in mind?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56624/new/

https://reviews.llvm.org/D56624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

Note that all of this currently only matters when compiling with 
`-fsanitize=unreachable`. The following discussion is within the context of the 
current implementation: UBSan removes the `noreturn` so it can instrument 
`unreachable` without the added instrumentation being optimized away. Maybe we 
should take a step back and ask if that is the right approach at all?

In D56624#1369795 , @vsk wrote:

> Because "expect_noreturn" calls are allowed to return, the compiler must 
> behave as they could. In particular, this means that unpoisoning the stack 
> before expect_noreturn calls (given the current semantics) is premature.
>
> Put another way, a frontend author may (understandably, but mistakenly!) 
> attach expect_noreturn to calls which they expect to be cold.


I think about this differently. Yes, most noreturn functions are also cold, 
e.g., `abort`, but not necessarily, e.g., calls to `longjmp` do not necessarily 
have to be. Why would it be okay to attach expect_noreturn instead of cold? Why 
do we think that this is an easy-to-make mistake? Have people accidentally put 
noreturn on cold functions before?
Can we agree on the following?
"It is orthogonal on the language level, but seems to be redundant in terms of 
the optimizer. Since LLVM IR's main purpose it support the optimizer, this is a 
strong argument against the general purpose attribute."

> That would regress ASan coverage.

You talk specifically about cases of misuses of the attribute, right?
In the context of the current issue with UBSan the possibility for false 
negative is not too much of a regression: it only occurs when UBSan is going to 
diagnose an "unreachable error" anyways.

So the main point is whether or not to use a "general purpose" attribute or a 
"narrow purpose" attribute/intrinsic. My understanding is that you list the 
following points as arguments against general purpose. Is my understanding 
accurate?

1. Potential misuse can regress ASan coverage
2. Complicates optimizer

Narrow purpose: No potential misuses, and optimizer can simply ignore it.

Initially I proposed a narrow purpose attribute, but while iterating on this 
revision changed it to be general purpose. @eugenis 
Now, I have a slight preference for general purpose: I don't think 1. is a big 
issue (but then again, I have no experience here),  and 2. it is always correct 
for the optimizer to continue ignoring the attribute (current status).
Actually, 2. also encompasses the potential upside: a more complicated 
optimizer that takes advantage of the attribute to do additional optimizations.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56624/new/

https://reviews.llvm.org/D56624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

In D56624#1369767 , @yln wrote:

> In D56624#1369635 , @vsk wrote:
>
> > What are the advantages of a generalized expect_noreturn attribute, vs. a 
> > narrower attribute or intrinsic? The expect_noreturn semantics do not 
> > provide strong guarantees, and are not really orthogonal from the 
> > pre-existing cold attribute.
>
>
> @eugenis Do you want to chime in here?
>  I think they convey different meanings even if their treatment by the 
> optimizer is similar. The `cold` attribute says nothing about whether or not 
> a function is expected to return.


That's my point: it doesn't need to, because it's orthogonal. It's just a hint 
that a call is cold and could be profitable to split/reorder. Features of llvm 
IR generally try to be orthogonal to reduce complexity in the optimizer.

>> In particular, expect_noreturn doesn't even seem strong enough to allow ASan 
>> to unpoison its stack.
> 
> I am not sure I understand this part. Can you elaborate?

Because "expect_noreturn" calls are allowed to return, the compiler must behave 
as they could. In particular, this means that unpoisoning the stack before 
expect_noreturn calls (given the current semantics) is premature.

Put another way, a frontend author may (understandably, but mistakenly!) attach 
expect_noreturn to calls which they expect to be cold. That would regress ASan 
coverage.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56624/new/

https://reviews.llvm.org/D56624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

@lebedev.ri
Thanks for the clarifications!
I will split this up into multiple patches once we settled on a design.

In D56624#1369635 , @vsk wrote:

> What are the advantages of a generalized expect_noreturn attribute, vs. a 
> narrower attribute or intrinsic? The expect_noreturn semantics do not provide 
> strong guarantees, and are not really orthogonal from the pre-existing cold 
> attribute.


@eugenis Do you want to chime in here?
I think they convey different meanings even if their treatment by the optimizer 
is similar. The `cold` attribute says nothing about whether or not a function 
is expected to return.

> In particular, expect_noreturn doesn't even seem strong enough to allow ASan 
> to unpoison its stack.

I am not sure I understand this part. Can you elaborate?

For context:

  ``cold``
This attribute indicates that this function is rarely called. When
computing edge weights, basic blocks post-dominated by a cold
function call are also considered to be cold; and, thus, given low
weight.
  ``noreturn``
This function attribute indicates that the function never returns
normally. This produces undefined behavior at runtime if the
function ever does dynamically return.
  ``expect_noreturn``
This function attribute indicates that the function is unlikely to return
normally, but that it is still allowed to do so. This is useful in cases
for which ``noreturn`` is too strong a guarantee.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56624/new/

https://reviews.llvm.org/D56624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

What are the advantages of a generalized expect_noreturn attribute, vs. a 
narrower attribute or intrinsic? The expect_noreturn semantics do not provide 
strong guarantees, and are not really orthogonal from the pre-existing cold 
attribute. In particular, expect_noreturn doesn't even seem strong enough to 
allow ASan to unpoison its stack.

Apologies if discussion has shifted elsewhere -- I'd be happy to chime in on 
the new review.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56624/new/

https://reviews.llvm.org/D56624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D56624#1369626 , @yln wrote:

> In D56624#1368966 , @lebedev.ri 
> wrote:
>
> > Please revert this.
> >  First, this wasn't reviewed.
> >  Second, the lists weren't subscribed.
>
>
> I apologize for this. It was not my intention to land the revision without 
> formal acceptance.
>
> commit-lists: 
>  I prepared this patch via the monorepo and did not select a repository in 
> Phabricator because the changes span multiple repos.


I guess rL  would be a safe default to 
make.

> This means that I have to manually ensure that the correct lists are 
> subscribed in the Phabricator web interface, correct?

Looks like the rules to subscribe the lists based on a content of the patch are 
still not added, so i'd say yes.

That being said, the normal practice in such situations is to open a new review.

Also, i suspect this should be split up into **at least** two parts - the new 
LLVM IR `expect_noreturn` attribute, and the rest.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56624/new/

https://reviews.llvm.org/D56624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Julian Lettner via Phabricator via cfe-commits]

yln reopened this revision.
yln added a comment.

In D56624#1368966 , @lebedev.ri wrote:

> Please revert this.
>  First, this wasn't reviewed.
>  Second, the lists weren't subscribed.


I apologize for this. It was not my intention to land the revision without 
formal acceptance.

commit-lists: 
I prepared this patch via the monorepo and did not select a repository in 
Phabricator because the changes span multiple repos. This means that I have to 
manually ensure that the correct lists are subscribed in the Phabricator web 
interface, correct?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56624/new/

https://reviews.llvm.org/D56624



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits