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

Allow .alt_entry symbols to pass the .cfi nesting check #82268

Merged
merged 3 commits into from
Feb 28, 2024

Conversation

jroelofs
Copy link
Contributor

@jroelofs jroelofs commented Feb 19, 2024

A symbol with an N_ALT_ENTRY attribute may be defined in the middle of a subsection, so it is reasonable to opt them out of the .cfi_{start,end}proc nesting check.

Fixes: #82261

@llvmbot llvmbot added backend:AArch64 mc Machine (object) code labels Feb 19, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 19, 2024

@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-aarch64

Author: Jon Roelofs (jroelofs)

Changes

Fixes: #82261


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

2 Files Affected:

  • (modified) llvm/lib/MC/MCParser/AsmParser.cpp (+4-1)
  • (modified) llvm/test/MC/AArch64/cfi-bad-nesting-darwin.s (+3-1)
diff --git a/llvm/lib/MC/MCParser/AsmParser.cpp b/llvm/lib/MC/MCParser/AsmParser.cpp
index 8e508dbdb1c69b..e8dc078c9610c2 100644
--- a/llvm/lib/MC/MCParser/AsmParser.cpp
+++ b/llvm/lib/MC/MCParser/AsmParser.cpp
@@ -44,6 +44,7 @@
 #include "llvm/MC/MCSection.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSymbol.h"
+#include "llvm/MC/MCSymbolMachO.h"
 #include "llvm/MC/MCTargetOptions.h"
 #include "llvm/MC/MCValue.h"
 #include "llvm/Support/Casting.h"
@@ -1950,7 +1951,9 @@ bool AsmParser::parseStatement(ParseStatementInfo &Info,
       Lex();
     }
 
-    if (MAI.hasSubsectionsViaSymbols() && CFIStartProcLoc && Sym->isExternal())
+    if (MAI.hasSubsectionsViaSymbols() && CFIStartProcLoc &&
+        Sym->isExternal() &&
+        (!isa<MCSymbolMachO>(Sym) || !cast<MCSymbolMachO>(Sym)->isAltEntry()))
       return Error(StartTokLoc, "non-private labels cannot appear between "
                                 ".cfi_startproc / .cfi_endproc pairs") &&
              Error(*CFIStartProcLoc, "previous .cfi_startproc was here");
diff --git a/llvm/test/MC/AArch64/cfi-bad-nesting-darwin.s b/llvm/test/MC/AArch64/cfi-bad-nesting-darwin.s
index 235b7d44809929..6a4dda3a77f05a 100644
--- a/llvm/test/MC/AArch64/cfi-bad-nesting-darwin.s
+++ b/llvm/test/MC/AArch64/cfi-bad-nesting-darwin.s
@@ -8,6 +8,8 @@
 	.p2align	2
 _locomotive:
 	.cfi_startproc
+	.alt_entry _engineer
+_engineer:
 	ret
 
 	; It is invalid to have a non-private label between .cfi_startproc and
@@ -17,7 +19,7 @@ _locomotive:
 	.p2align	2
 _caboose:
 ; DARWIN: [[#@LINE-1]]:1: error: non-private labels cannot appear between .cfi_startproc / .cfi_endproc pairs
-; DARWIN: [[#@LINE-10]]:2: error: previous .cfi_startproc was here
+; DARWIN: [[#@LINE-12]]:2: error: previous .cfi_startproc was here
 	ret
 	.cfi_endproc
 

if (MAI.hasSubsectionsViaSymbols() && CFIStartProcLoc && Sym->isExternal())
if (MAI.hasSubsectionsViaSymbols() && CFIStartProcLoc &&
Sym->isExternal() &&
(!isa<MCSymbolMachO>(Sym) || !cast<MCSymbolMachO>(Sym)->isAltEntry()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since only MCAsmInfoDarwin sets hasSubsectionsViaSymbols. The isa<MCSymbolMachO>(Sym) check is unneeded.

Copy link
Member

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes: #82261

Improve the description by saying that a symbol with the N_ALT_ENTRY attribute can be defined in the middle of a subsection. Therefore it is reasonable to opt out the .cfi_startproc nesting check?

@@ -8,6 +8,8 @@
.p2align 2
_locomotive:
.cfi_startproc
.alt_entry _engineer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment that a N_ALT_ENTRY symbol can be in the middle of a subsection?

@MaskRay
Copy link
Member

MaskRay commented Feb 24, 2024

This is a good candidate for release/18.x

@jroelofs jroelofs merged commit 5b91647 into llvm:main Feb 28, 2024
4 checks passed
@jroelofs
Copy link
Contributor Author

/cherry-pick 5b91647

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 28, 2024

/cherry-pick 5b91647

Error: Command failed due to missing milestone.

@jroelofs jroelofs added this to the LLVM 18.X Release milestone Feb 28, 2024
@jroelofs
Copy link
Contributor Author

/cherry-pick 5b91647

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 28, 2024
A symbol with an `N_ALT_ENTRY` attribute may be defined in the middle of
a subsection, so it is reasonable to opt them out of the
`.cfi_{start,end}proc` nesting check.

Fixes: llvm#82261
(cherry picked from commit 5b91647)
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 28, 2024

/pull-request #83336

ForsakenHarmony pushed a commit to ForsakenHarmony/llvm-project that referenced this pull request Feb 29, 2024
A symbol with an `N_ALT_ENTRY` attribute may be defined in the middle of
a subsection, so it is reasonable to opt them out of the
`.cfi_{start,end}proc` nesting check.

Fixes: llvm#82261
(cherry picked from commit 5b91647)
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Mar 11, 2024
A symbol with an `N_ALT_ENTRY` attribute may be defined in the middle of
a subsection, so it is reasonable to opt them out of the
`.cfi_{start,end}proc` nesting check.

Fixes: llvm#82261
(cherry picked from commit 5b91647)
@pointhex pointhex mentioned this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AArch64 mc Machine (object) code
Projects
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
3 participants