Skip to content
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.

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

[MC] Don't treat altentry symbols as atoms #97479

Merged

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Jul 2, 2024

The current setAtom is inaccurate: a .alt_entry label can also be
recognized as an atom. This is mostly benign, but might cause two
locations only separated by an .alt_entry to have different atoms.

https://reviews.llvm.org/D153167 changed a evaluateKnownAbsolute to
evaluateAsAbsolute and would not fold A-B even if they are only
separated by a .alt_entry label, leading to a spurious error
invalid CFI advance_loc expression.

The fix is similar to #82268: add a special case for .alt_entry.

Fix #97116

Created using spr 1.3.5-bogner
@llvmbot llvmbot added the mc Machine (object) code label Jul 2, 2024
@MaskRay MaskRay requested review from jroelofs and aengelke July 2, 2024 20:55
@llvmbot
Copy link
Member

llvmbot commented Jul 2, 2024

@llvm/pr-subscribers-mc

Author: Fangrui Song (MaskRay)

Changes

The current setAtom is inaccurate: a .alt_entry label can also be
recognized as an atom. This is mostly benign, but might cause two
locations only separated by an .alt_entry to have different atoms.

The evaluateAsAbsolute code added by https://reviews.llvm.org/D153167
would not fold A-B when they are only separated by a .alt_entry
label, leading to a spurious error.

The fix is similar to #82268: add a special case for .alt_entry.

Fix #97116


Full diff: https://github.com/llvm/llvm-project/pull/97479.diff

2 Files Affected:

  • (modified) llvm/lib/MC/MCMachOStreamer.cpp (+1-1)
  • (modified) llvm/test/MC/MachO/cfi-advance-loc-err.s (+5)
diff --git a/llvm/lib/MC/MCMachOStreamer.cpp b/llvm/lib/MC/MCMachOStreamer.cpp
index d67b95820a8a9..0b34d87033b7b 100644
--- a/llvm/lib/MC/MCMachOStreamer.cpp
+++ b/llvm/lib/MC/MCMachOStreamer.cpp
@@ -508,7 +508,7 @@ void MCMachOStreamer::finishImpl() {
   DenseMap<const MCFragment *, const MCSymbol *> DefiningSymbolMap;
   for (const MCSymbol &Symbol : getAssembler().symbols()) {
     if (getAssembler().isSymbolLinkerVisible(Symbol) && Symbol.isInSection() &&
-        !Symbol.isVariable()) {
+        !Symbol.isVariable() && !cast<MCSymbolMachO>(Symbol).isAltEntry()) {
       // An atom defining symbol should never be internal to a fragment.
       assert(Symbol.getOffset() == 0 &&
              "Invalid offset in atom defining symbol!");
diff --git a/llvm/test/MC/MachO/cfi-advance-loc-err.s b/llvm/test/MC/MachO/cfi-advance-loc-err.s
index 3143dd84efc63..77b6544cb12d8 100644
--- a/llvm/test/MC/MachO/cfi-advance-loc-err.s
+++ b/llvm/test/MC/MachO/cfi-advance-loc-err.s
@@ -9,6 +9,11 @@ _foo:
   subq $8, %rsp
   .cfi_adjust_cfa_offset 8
 
+  .alt_entry _bar
+_bar: # alt_entry label can appear here as it is not an atom
+  addq $8, %rsp
+  .cfi_adjust_cfa_offset -8
+
 tmp0: # non-private label cannot appear here
   addq $8, %rsp
 # CHECK: :[[#@LINE+1]]:3: error: invalid CFI advance_loc expression

Created using spr 1.3.5-bogner
@MaskRay MaskRay merged commit 21276fd into main Jul 2, 2024
4 of 6 checks passed
@MaskRay MaskRay deleted the users/MaskRay/spr/mc-dont-treat-altentry-symbols-as-atoms branch July 2, 2024 22:19
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
The current `setAtom` is inaccurate: a `.alt_entry` label can also be
recognized as an atom. This is mostly benign, but might cause two
locations only separated by an `.alt_entry` to have different atoms.

https://reviews.llvm.org/D153167 changed a `evaluateKnownAbsolute` to
`evaluateAsAbsolute` and would not fold `A-B` even if they are only
separated by a `.alt_entry` label, leading to a spurious error
`invalid CFI advance_loc expression`.

The fix is similar to llvm#82268: add a special case for `.alt_entry`.

Fix llvm#97116

Pull Request: llvm#97479
kbluck pushed a commit to kbluck/llvm-project that referenced this pull request Jul 6, 2024
The current `setAtom` is inaccurate: a `.alt_entry` label can also be
recognized as an atom. This is mostly benign, but might cause two
locations only separated by an `.alt_entry` to have different atoms.

https://reviews.llvm.org/D153167 changed a `evaluateKnownAbsolute` to
`evaluateAsAbsolute` and would not fold `A-B` even if they are only
separated by a `.alt_entry` label, leading to a spurious error
`invalid CFI advance_loc expression`.

The fix is similar to llvm#82268: add a special case for `.alt_entry`.

Fix llvm#97116

Pull Request: llvm#97479
@filipnavara
Copy link

JFYI The original issue is somehow present in Xcode 16 beta 2/3 which is based on LLVM 17 AFAICT. I filed Apple feedback to get appropriate backport of the fix (FB14191360).

@MaskRay
Copy link
Member Author

MaskRay commented Jul 10, 2024

JFYI The original issue is somehow present in Xcode 16 beta 2/3 which is based on LLVM 17 AFAICT. I filed Apple feedback to get appropriate backport of the fix (FB14191360).

Thank you. I am glad that this resolved an issue for you:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mc Machine (object) code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mach-O check for improperly nested .cfi_* regions doesn't take .alt_entry into account
4 participants