Remove `-funique-traps` and replace it with a better implementation (rdar://158088410) #11455

delcypher · 2025-09-18T20:53:27Z

This PR does ~~three~~ two things (each done as a separate commit).

Removes the old -funique-traps flag and implementation. See the commit that does this for a list of the problems with that implementation.
Adds a -fbounds-safety-unique-traps flag that uses a completely different implementation to -funique-traps. The implementation is better because

It doesn't make use of fake asm instructions in trap block. These caused problems with the hot-cold splitting pass and also made LLVM IR harder to read.
It uses a mechanism already implemented upstream for UBSan traps (the nomerge attribute on calls to the trap intrinsic). So the implementation here is much less likely to conflict with upstream.

3. Adds a unique_traps Clang attribute that can be used to control trap merging at a per-function level. This is intended to be used in situations where code size is really important where it is not feasible to disable trap merging everywhere. The attributes interface is deliberately made generic so that it can be re-used by other compiler instrumentation that emits trap instructions. This is something I will likely try to upstream.

rdar://158088410

delcypher · 2025-09-18T20:53:53Z

@swift-ci test llvm

delcypher · 2025-09-18T23:35:22Z

@swift-ci test llvm

delcypher · 2025-09-22T19:02:51Z

 1 warning(s) in tests
18:14:21  ********************
18:14:21  Failed Tests (4):
18:14:21    Clang :: Misc/pragma-attribute-supported-attributes-list.test
18:14:21    Clang :: Misc/warning-flags.c
18:14:21    Clang-Unit :: ./AllClangUnitTests/DependencyScanningCAPITests/DependencyScanningFSCacheOutOfDate
18:14:21    LLVM :: Transforms/PhaseOrdering/X86/excessive-unrolling.ll

The Misc/pragma-attribute-supported-attributes-list.test test failure was my PR. I don't think the other failures are related. I've rebased and I'll re-run the tests.

delcypher · 2025-09-22T19:03:00Z

@swift-ci test llvm

hnrklssn

I don't quite follow why the unique_traps attribute needs this extra complexity, or why we need to specify -fbounds-safety-unique-traps instead of just -unique-traps etc. Can't we just have one flag/attribute that applies to both bounds safety and ubsan?

hnrklssn · 2025-09-22T22:58:56Z

clang/test/BoundsSafety/CodeGen/unique-trap-blocks-O0-O2-opt-disabled.c

 // OPT0-NEXT:    [[TMP2:%.*]] = icmp ule ptr [[TMP1]], [[WIDE_PTR_UB]], !annotation [[META2]]
 // OPT0-NEXT:    br i1 [[TMP2]], label %[[CONT:.*]], label %[[TRAP:.*]], !prof [[PROF3:![0-9]+]], !annotation [[META2]]
 // OPT0:       [[TRAP]]:
-// OPT0-NEXT:    call void asm sideeffect "", "n"(i64 0), !annotation [[META2]]


Why are these calls removed now?

These were never really wanted in the first place. The original -funique-traps added them because that's how Swift "uniqued" its traps and prevented the backend from merging them.

I took a different approach so these shouldn't be needed anymore.

hnrklssn · 2025-09-22T23:00:15Z

clang/test/BoundsSafety/CodeGen/unique-traps-O2.c

+  int other = i_want_to_be_inlined(ptr2, idx);
+  return ptr[idx];
+}
+//


Are these extra comments emitted by UTC? Seems like a bug imo

If you mean the ones after the closing brace of consume then I think so.

delcypher · 2025-09-22T23:17:55Z

@hnrklssn Good questions. Sorry I didn't explain this clearly enough.

I don't quite follow why the unique_traps attribute needs this extra complexity, or why we need to specify -fbounds-safety-unique-traps instead of just -unique-traps etc. Can't we just have one flag/attribute that applies to both bounds safety and ubsan?

Can't we just have one flag/attribute that applies to both bounds safety and ubsan?

UBSan already has -fsanitize-merge= (e.g. -fsanitize-merge=undefined) and it didn't make much sense to me to force ourselves into this flag. That requires agreement from upstream and also the flag name doesn't make much sense for us because -fbounds-safety is not a sanitizer.

why we need to specify -fbounds-safety-unique-traps instead of just -unique-traps

-funique-traps has been removed. My original idea was that it should apply to all traps but because upstream already added their own flag and made it sanitizer specific it made me think it didn't really make much sense to have a flag called -funique-traps because in the implementation it would only apply to -fbounds-safety. Instead I've added a flag which is -fbounds-safety specific to complement the existing upstream -fsanitize-merge= flag.

I don't quite follow why the unique_traps attribute needs this extra complexity

I was trying to make the attribute as general as possible so that it can be upstreamed. If I make it -fbounds-safety specific the motivation for upstreaming it is pretty weak because it doesn't do anything.

hnrklssn · 2025-09-23T00:20:07Z

UBSan already has -fsanitize-merge=

Do we want to mirror this by naming this flag something like -fbounds-safety-merge (with the default being enabled)? I personally don't have a strong opinion here, but it struck me as odd that these strongly related flags were named completely differently.

I was trying to make the attribute as general as possible so that it can be upstreamed. If I make it -fbounds-safety specific the motivation for upstreaming it is pretty weak because it doesn't do anything.

The attribute doesn't necessarily have to mirror the flags. It could still be [[clang::unique_traps]] and apply to both bounds safety and ubsan traps, for simplicity. Unless not being able to specify specific sanitizers is a blocker for upstreaming, that is. Do we know whether that is the case?

Right now it's constructed generically, but there's only one acceptable use. If upstreamability is important, I suggest implementing the attribute in upstream first: that way it's black on white whether the design of the attribute is actually acceptable for upstream Clang.

delcypher · 2025-09-23T18:08:06Z

@hnrklssn

Do we want to mirror this by naming this flag something like -fbounds-safety-merge (with the default being enabled)? I personally don't have a strong opinion here, but it struck me as odd that these strongly related flags were named completely differently.

Not really. I think the name in this PR is a lot clearer than fbounds-safety-merge because that doesn't say what's being merged. If you wanted something clearer it should probably be named -fbounds-safety-merge-traps.

The attribute doesn't necessarily have to mirror the flags. It could still be [[clang::unique_traps]] and apply to both bounds safety and ubsan traps, for simplicity.

While that's true I did this to simplify the implementation and reduce the chance of a merge conflict because all the logic is in CodeGenFunction::EmitBoundsSafetyTrapCheck instead of CodeGenFunction::EmitTrapCheck.

What you propose does simplify things from a user perspective though.

Unless not being able to specify specific sanitizers is a blocker for upstreaming, that is. Do we know whether that is the case?

We do not know because we have not approached upstream with this design yet.

Right now it's constructed generically, but there's only one acceptable use. If upstreamability is important, I suggest implementing the attribute in upstream first: that way it's black on white whether the design of the attribute is actually acceptable for upstream Clang.

It's a nice to have. For the sake of time constraints I'd much rather land what we have now and upstream this after and amend the design as necessary because upstreaming stuff is almost always very slow.

delcypher · 2025-09-23T18:08:19Z

@swift-ci test llvm

hnrklssn · 2025-09-23T18:33:30Z

While that's true I did this to simplify the implementation and reduce the chance of a merge conflict because all the logic is in CodeGenFunction::EmitBoundsSafetyTrapCheck instead of CodeGenFunction::EmitTrapCheck.

We could still have it apply only in EmitBoundsSafetyTrapCheck until upstreamed, if that's a concern. Or we could make the attribute explicitly bounds safety specific.

What you propose does simplify things from a user perspective though.

The reason I push for this is that these attributes are not a write-once kind of thing, but only used during debugging. Making them quick to write, easy to remember, and hard to use incorrectly lowers the friction to actually use them.

We do not know because we have not approached upstream with this design yet.

I'm not a huge fan of guessing what upstream wants.

It's a nice to have. For the sake of time constraints I'd much rather land what we have now and upstream this after and amend the design as necessary

If we don't know whether we'll upstream it, we could just as well tailor it for our specific use case and amend that design afterwards.

because upstreaming stuff is almost always very slow.

It can be slow, but not necessarily - especially for small, targeted things.

delcypher · 2025-09-23T21:41:50Z

The reason I push for this is that these attributes are not a write-once kind of thing, but only used during debugging. Making them quick to write, easy to remember, and hard to use incorrectly lowers the friction to actually use them.

If that's all you're concerned about we can side step a lot of this problem by putting a macro in ptrcheck.h

#define __bounds_safety_unique_traps __attribute__((unique_traps("bounds-safety")))

rapidsna · 2025-09-26T00:59:20Z

clang/include/clang/Basic/Attr.td

 }
+
+def UniqueTrap : InheritableAttr {
+  let Spellings = [Clang<"unique_traps">];


How are you imagining UniqueTrapKind will evolve when you get to upstream it? Are you imagining UBSan traps will be controlled by each individual trap kind?

The simplest version would be to have a single enum value for UBSan. A more complicated version would be to allow fine grained control of the different UBSan trap kinds so that it mirrors the behavior of the existing compiler flag.

I will start the process of trying to upstream the attribute but I don’t really want to block this PR on doing that.

Do we need the attributes to land in the builds urgently? Alternatively, you could land the first two commits first unless attributes are urgent. The benefit would be then we don't need to deal with diverging changes there after upstreaming, e.g., if the attribute name has to change or implementation should be different.

We don't so I'll land the first two commits, and work on the commit that adds the attribute in open source.

The implementation was a bit hacky and interacted badly with the hot-cold splitting pass. So it is going to be removed in favor of a different implementation that's simpler. I also investated why `clang/test/BoundsSafety/CodeGen/unique-trap-blocks-O2.c` broke (rdar://150559639). It turns outthat the "trap blocks" started being unintentionally split by the hot-cold splitting pass because in upstream we started adding branch profiling weights (llvm#134310) which the hot-cold splitting pass used as a signal to split the "trap blocks" into their own function (by-passing the logic I originally added to prevent the splitting). While we could fix this it would be very fragile and given that we are removing the implementation anyway it doesn't matter anymore. rdar://158088410

This patch adds a driver/cc1 flag that prevents `-fbounds-safety` traps from being merged. This improves debuggability (because now it is possible to see which check lead to the trap) at the cost of increased code size. It is currently off by default. This implementation adds the `nomerge` attribute on call instructions to `-fbounds-safety` traps at the LLVM IR level and prevents re-using trap blocks during Clang's IRGen when the compiler flag is passed. This was basically already implemented upstream for trapping UBSan which meant the only thing we needed to do was set the `NoMerge` parameter to `true` when calling `EmitTrapCheck`. This is the replacement for the recently removed `-funique-traps` which was less than ideal for several reasons: * The use of `asm` instructions in the trap blocks made analyzing LLVM IR more challenging than it needed to be. * The trap blocks interacted poorly with the hot-cold splitting pass and required fragile workarounds to prevent the trap blocks from being split into their own function. The implementation of `-fbound-safety-unique-traps` has none of these problems. rdar://158088410

delcypher · 2025-10-01T00:05:24Z

@swift-ci test llvm

delcypher · 2025-10-01T04:01:36Z

The macOS test failures are known existing issues:

18:52:56  ********************
18:52:56  Unresolved Tests (3):
18:52:56    Clang :: Index/Store/json-with-module.m
18:52:56    Clang :: Index/Store/json-with-pch.c
18:52:56    Clang :: Index/Store/syntax-only.c
18:52:56  
18:52:56  ********************
18:52:56  Failed Tests (8):
18:52:56    Clang :: BoundsSafety/AST/attributes_in_template_decls_attr_only_mode.cpp
18:52:56    Clang :: BoundsSafety/CodeGen/arithmetic-ops-in-counted-by-assign-O2.c
18:52:56    LLVM :: Transforms/Coroutines/coro-retcon-frame-old.ll
18:52:56    LLVM :: Transforms/Coroutines/coro-retcon-once-dynamic-nocleanup.ll
18:52:56    LLVM :: Transforms/Coroutines/coro-retcon-once-dynamic.ll
18:52:56    LLVM :: Transforms/Coroutines/coro-retcon-swift-coroFrameAlloc.ll
18:52:56    LLVM :: Transforms/Coroutines/coro-split-swift_coroFrameAlloc.ll
18:52:56    LLVM :: Transforms/PhaseOrdering/X86/excessive-unrolling.ll

delcypher requested review from fhahn, jroelofs, hnrklssn, rapidsna, patrykstefanski and dtellenbach September 18, 2025 20:53

delcypher self-assigned this Sep 18, 2025

delcypher added the clang:bounds-safety Issue relating to the experimental -fbounds-safety feature in Clang label Sep 18, 2025

delcypher requested a review from AbhayKanhere September 18, 2025 20:55

delcypher force-pushed the dliew/rdar/158088410 branch from d500767 to 1a7169d Compare September 18, 2025 23:34

delcypher force-pushed the dliew/rdar/158088410 branch from 1a7169d to 2752ea9 Compare September 22, 2025 18:59

hnrklssn reviewed Sep 22, 2025

View reviewed changes

rapidsna reviewed Sep 26, 2025

View reviewed changes

delcypher added 2 commits September 30, 2025 17:02

delcypher force-pushed the dliew/rdar/158088410 branch from 2752ea9 to 5890357 Compare October 1, 2025 00:03

rapidsna approved these changes Oct 1, 2025

View reviewed changes

delcypher merged commit 9b1d55c into swiftlang:next Oct 1, 2025
0 of 2 checks passed

delcypher mentioned this pull request Oct 1, 2025

🍒 Cherry-pick "Remove -funique-traps and replace it with a better implementation (rdar://158088410)" #11548

Merged

Remove -funique-traps and replace it with a better implementation (rdar://158088410) #11455

Remove -funique-traps and replace it with a better implementation (rdar://158088410) #11455

Uh oh!

Conversation

delcypher commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

delcypher commented Sep 18, 2025

Uh oh!

delcypher commented Sep 18, 2025

Uh oh!

delcypher commented Sep 22, 2025

Uh oh!

delcypher commented Sep 22, 2025

Uh oh!

hnrklssn left a comment

Choose a reason for hiding this comment

Uh oh!

hnrklssn Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

delcypher Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

hnrklssn Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

delcypher Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

hnrklssn Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

delcypher commented Sep 22, 2025

Uh oh!

hnrklssn commented Sep 23, 2025

Uh oh!

delcypher commented Sep 23, 2025

Uh oh!

delcypher commented Sep 23, 2025

Uh oh!

hnrklssn commented Sep 23, 2025

Uh oh!

delcypher commented Sep 23, 2025

Uh oh!

rapidsna Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delcypher Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

rapidsna Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delcypher Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

delcypher commented Oct 1, 2025

Uh oh!

delcypher commented Oct 1, 2025

Uh oh!

Uh oh!

Uh oh!

Remove `-funique-traps` and replace it with a better implementation (rdar://158088410) #11455

Remove `-funique-traps` and replace it with a better implementation (rdar://158088410) #11455

delcypher commented Sep 18, 2025 •

edited

Loading

rapidsna Sep 26, 2025 •

edited

Loading

rapidsna Sep 30, 2025 •

edited

Loading