Skip to content

Make [[clang::nomerge]] work for trap intrinsics such as __debugbreak and __builtin_trap #53011

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
rnk opened this issue Jan 5, 2022 · 2 comments · Fixed by #101549
Closed

Make [[clang::nomerge]] work for trap intrinsics such as __debugbreak and __builtin_trap #53011

rnk opened this issue Jan 5, 2022 · 2 comments · Fixed by #101549
Assignees
@ZequanWu
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc.

Comments

@rnk
Copy link
Collaborator

rnk commented Jan 5, 2022

Consider:
https://gcc.godbolt.org/z/esvPqdfn9

#define NOMERGE [[clang::nomerge]]
bool condition();
void mergeChecks() {
    if (condition())
      NOMERGE __builtin_trap();
      if (condition())
      NOMERGE __builtin_trap();
      if (condition())
      NOMERGE __builtin_trap();
}

LLVM folds all these traps together:

_Z11mergeChecksv: # @_Z11mergeChecksv
  push rax
  call _Z9conditionv
  test al, al
  jne .LBB0_4
  call _Z9conditionv
  test al, al
  jne .LBB0_4
  call _Z9conditionv
  test al, al
  jne .LBB0_4
  pop rax
  ret
.LBB0_4: # %if.then
  ud2

Ideally, the nomerge attribute should prevent this. Currently the attribute only really works with call expressions.
@rnk rnk added the clang:codegen IR generation bugs: mangling, exceptions, etc. label Jan 5, 2022
@davidben
Copy link
Contributor

We currently use an inline asm block in Chromium, basically to work around not being able to say this in CHECKs. If [[clang::nomerge]] __builtin_trap() worked, I suspect we could switch to that!

@rnk
Copy link
Collaborator Author

rnk commented Feb 9, 2024

@ZequanWu started work on this in https://reviews.llvm.org/D146164 , but I think it stalled out over the concern of extending support to all the other builtins which generate calls (__builtin_printf, etc).

@ZequanWu ZequanWu mentioned this issue Aug 1, 2024
Merged
ZequanWu added a commit that referenced this issue Aug 1, 2024
@ZequanWu
[Clang] Fix nomerge attribute not working with __builtin_trap(), __de…
5e84646 
…bugbreak(), __builtin_verbose_trap() (#101549)

1. It fixes the problem that llvm.trap() not getting the nomerge
attribute.
2. It sets nomerge flag for the node if the instruction has nomerge
arrtibute.

This is a copy of https://reviews.llvm.org/D146164. This only attempts
to fix `nomerge` for `__builtin_trap()`, `__debugbreak()`,
`__builtin_verbose_trap()`, not working for non-trap builtins.

Fixes #53011
thurstond added a commit to thurstond/llvm-project that referenced this issue Nov 26, 2024
@thurstond
[NFC][clang] Add ubsan-trap-merge.c test to show absence of nomerge
d85a763 
This test demonstrates that UBSan does not add the nomerge annotation. This is significant because it results in them being merged by the backend.

N.B. llvm#65972 (continuation of https://reviews.llvm.org/D148654) had considered adding nomerge to ubsantrap, but did not proceed with that because of llvm#53011. llvm#101549 fixed that limitation ("It sets nomerge flag for the node if the instruction has nomerge arrtibute."); planned upcoming will add nomerge for ubsan.
@thurstond thurstond mentioned this issue Nov 26, 2024
Merged
thurstond added a commit to thurstond/llvm-project that referenced this issue Nov 26, 2024
@thurstond
[ubsan] Change ubsan-unique-traps to use nomerge instead of counter
f24a87d 
llvm#65972 (continuation of https://reviews.llvm.org/D148654) had considered adding nomerge to ubsantrap, but did not proceed with that because of llvm#53011. Instead, it added a counter (based on TrapBB->getParent()->size()) to each ubsantrap call. However, this counter is not guaranteed to be unique after inlining, as shown by llvm#83470, which can result in ubsantraps being merged by the backend.

llvm#101549 fixed has since fixed the nomerge limitation ("It sets nomerge flag for the node if the instruction has nomerge arrtibute."). This patch therefore takes advantage of nomerge instead of using the counter, guaranteeing that the ubsantraps are not merged.

This patch is equivalent to llvm#83470 but also adds nomerge and updates the test that was precommitted in llvm#117649.
@thurstond thurstond mentioned this issue Nov 26, 2024
Merged
thurstond added a commit that referenced this issue Nov 26, 2024
@thurstond
[NFC][clang] Add ubsan-trap-merge.c test to show absence of nomerge (#…
1b68b33 
…117649)

This test (copied from #83470)
demonstrates that UBSan does not add the nomerge annotation. This is
significant because it can result in them being merged by the backend,
even when -ubsan-unique-traps is enabled.

N.B. #65972 (continuation of
https://reviews.llvm.org/D148654) had considered adding nomerge to
ubsantrap, but did not proceed with that because of
#53011.
#101549 fixed that limitation
("It sets nomerge flag for the node if the instruction has nomerge
arrtibute."); planned upcoming work
(#117651) will add nomerge for
ubsan.
thurstond added a commit to thurstond/llvm-project that referenced this issue Nov 26, 2024
@thurstond
[ubsan] Change ubsan-unique-traps to use nomerge instead of counter
cffa7c3 
llvm#65972 (continuation of https://reviews.llvm.org/D148654) had considered adding nomerge to ubsantrap, but did not proceed with that because of llvm#53011. Instead, it added a counter (based on TrapBB->getParent()->size()) to each ubsantrap call. However, this counter is not guaranteed to be unique after inlining, as shown by llvm#83470, which can result in ubsantraps being merged by the backend.

llvm#101549 fixed has since fixed the nomerge limitation ("It sets nomerge flag for the node if the instruction has nomerge arrtibute."). This patch therefore takes advantage of nomerge instead of using the counter, guaranteeing that the ubsantraps are not merged.

This patch is equivalent to llvm#83470 but also adds nomerge and updates the test that was precommitted in llvm#117649.
thurstond added a commit to thurstond/llvm-project that referenced this issue Nov 26, 2024
@thurstond
Reapply "[NFC][clang] Add ubsan-trap-merge.c test to show absence of …
b5628ad 
…nomerge" (llvm#117804)

This reverts commit c8bdb31.

It was reverted because I forgot to update the auto-generated assertions after adding the target triple.

Original commit message:

This test (copied from llvm#83470) demonstrates that UBSan does not add the nomerge annotation. This is significant because it can result in them being merged by the backend, even when -ubsan-unique-traps is enabled.

N.B. llvm#65972 (continuation of https://reviews.llvm.org/D148654) had considered adding nomerge to ubsantrap, but did not proceed with that because of llvm#53011. llvm#101549 fixed that limitation ("It sets nomerge flag for the node if the instruction has nomerge arrtibute."); planned upcoming work (llvm#117651) will add nomerge for ubsan.
@thurstond thurstond mentioned this issue Nov 26, 2024
Merged
thurstond added a commit that referenced this issue Nov 26, 2024
@thurstond
Reapply "[NFC][clang] Add ubsan-trap-merge.c test to show absence of …
47dbf35 
…nomerge" (#117804) (#117805)

This reverts commit c8bdb31.

It was reverted because I forgot to update the auto-generated assertions
after adding the target triple.

Original commit message:

This test (copied from #83470)
demonstrates that UBSan does not add the nomerge annotation. This is
significant because it can result in them being merged by the backend,
even when -ubsan-unique-traps is enabled.

N.B. #65972 (continuation of
https://reviews.llvm.org/D148654) had considered adding nomerge to
ubsantrap, but did not proceed with that because of
#53011.
#101549 fixed that limitation
("It sets nomerge flag for the node if the instruction has nomerge
arrtibute."); planned upcoming work
(#117651) will add nomerge for
ubsan.
thurstond added a commit to thurstond/llvm-project that referenced this issue Nov 26, 2024
@thurstond
[ubsan] Change ubsan-unique-traps to use nomerge instead of counter
e635c76 
llvm#65972 (continuation of https://reviews.llvm.org/D148654) had considered adding nomerge to ubsantrap, but did not proceed with that because of llvm#53011. Instead, it added a counter (based on TrapBB->getParent()->size()) to each ubsantrap call. However, this counter is not guaranteed to be unique after inlining, as shown by llvm#83470, which can result in ubsantraps being merged by the backend.

llvm#101549 fixed has since fixed the nomerge limitation ("It sets nomerge flag for the node if the instruction has nomerge arrtibute."). This patch therefore takes advantage of nomerge instead of using the counter, guaranteeing that the ubsantraps are not merged.

This patch is equivalent to llvm#83470 but also adds nomerge and updates the test that was precommitted in llvm#117649.
thurstond added a commit that referenced this issue Nov 27, 2024
@thurstond
[ubsan] Change ubsan-unique-traps to use nomerge instead of counter (#…
0d15d46 
…117651)

#65972 (continuation of
https://reviews.llvm.org/D148654) had considered adding nomerge to
ubsantrap, but did not proceed with that because of
#53011. Instead, it added a
counter (based on TrapBB->getParent()->size()) to each ubsantrap call.
However, this counter is not guaranteed to be unique after inlining, as
shown by #83470, which can
result in ubsantraps being merged by the backend.

#101549 has since fixed the
nomerge limitation ("It sets nomerge flag for the node if the
instruction has nomerge arrtibute."). This patch therefore takes
advantage of nomerge instead of using the counter, guaranteeing that the
ubsantraps are not merged.

This patch is equivalent to
#83470 but also adds nomerge
and updates tests (#117649:
ubsan-trap-merge.c; #117657:
ubsan-trap-merge.ll, ubsan-trap-nomerge.ll; catch-undef-behavior.c).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ZequanWu ZequanWu

Labels
clang:codegen IR generation bugs: mangling, exceptions, etc.
Projects
None yet
Milestone
No milestone
3 participants
@rnk @davidben @ZequanWu