Skip to content

[MC] Parse SHF_LINK_ORDER argument before section group name #77407

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
Merged
merged 1 commit into from
Jan 9, 2024
Merged

[MC] Parse SHF_LINK_ORDER argument before section group name #77407

merged 1 commit into from
Jan 9, 2024

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Jan 9, 2024 •
edited
Loading

When both SHF_LINK_ORDER | SHF_GROUP flags are set, GNU assembler from
2.35 onwards (https://sourceware.org/PR25381
https://sourceware.org/binutils/docs/as/Section.html) parses the
SHF_LINK_ORDER argument before section group name, different from us.

This is unfortunate, but does not matter because the .section flag o
is a niche feature only used by compiler instrumentations, not adopted
by hand-written assembly, and using both flags is extremely rare. Let's
just match GNU assembler. There is another benefit: we now support
zero-flag section group with the SHF_LINK_ORDER flag, while previously
there isn't a syntax.

While here, print 'G' after 'o' to be clear that the 'G' argument is
parsed after the 'o' argument. To make the diff smaller, we don't print
'G' after 'w' in the absence of 'o' for now.

@llvmbot
Copy link
Member

llvmbot commented Jan 9, 2024 •
edited
Loading

@llvm/pr-subscribers-backend-aarch64
@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-loongarch

Author: Fangrui Song (MaskRay)

Changes

When both SHF_LINK_ORDER | SHF_GROUP flags are set, GNU assembler from
2.35 onwards (https://sourceware.org/PR25381
https://sourceware.org/binutils/docs/as/Section.html) parses the
SHF_LINK_ORDER argument before section group name, different from us.

This is unfortunate, but does not matter because the .section flag o
is a niche feature only used by compiler instrumentations, not adopted
by hand-written assembly, and using both flags is extremely rare. Let's
just match GNU assembler. While here, print 'G' after 'o' to be clear
that the 'G' argument is parsed after the 'o' argument. To make the diff
smaller, we don't print 'G' after 'w' in the absence of 'o' for now.

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

13 Files Affected:

  • (modified) llvm/lib/MC/MCParser/ELFAsmParser.cpp (+3-3)
  • (modified) llvm/lib/MC/MCSectionELF.cpp (+12-8)
  • (modified) llvm/test/CodeGen/AArch64/patchable-function-entry.ll (+2-2)
  • (modified) llvm/test/CodeGen/LoongArch/patchable-function-entry.ll (+1-1)
  • (modified) llvm/test/CodeGen/Mips/xray-section-group.ll (+1-1)
  • (modified) llvm/test/CodeGen/RISCV/patchable-function-entry.ll (+1-1)
  • (modified) llvm/test/CodeGen/X86/basic-block-sections-labels-functions-sections.ll (+1-1)
  • (modified) llvm/test/CodeGen/X86/gcc_except_table-multi.ll (+2-2)
  • (modified) llvm/test/CodeGen/X86/patchable-function-entry.ll (+2-2)
  • (modified) llvm/test/CodeGen/X86/stack-size-section-function-sections.ll (+2-2)
  • (modified) llvm/test/CodeGen/X86/stack-size-section.ll (+1-1)
  • (modified) llvm/test/CodeGen/X86/xray-section-group.ll (+1-1)
  • (modified) llvm/test/MC/ELF/section-combine.s (+3-3)
diff --git a/llvm/lib/MC/MCParser/ELFAsmParser.cpp b/llvm/lib/MC/MCParser/ELFAsmParser.cpp
index 93e1d2f44b8c56..d4c4bcb8564889 100644
--- a/llvm/lib/MC/MCParser/ELFAsmParser.cpp
+++ b/llvm/lib/MC/MCParser/ELFAsmParser.cpp
@@ -616,12 +616,12 @@ bool ELFAsmParser::ParseSectionArguments(bool IsPush, SMLoc loc) {
     if (Mergeable)
       if (parseMergeSize(Size))
         return true;
-    if (Group)
-      if (parseGroup(GroupName, IsComdat))
-        return true;
     if (Flags & ELF::SHF_LINK_ORDER)
       if (parseLinkedToSym(LinkedToSym))
         return true;
+    if (Group)
+      if (parseGroup(GroupName, IsComdat))
+        return true;
     if (maybeParseUniqueID(UniqueID))
       return true;
   }
diff --git a/llvm/lib/MC/MCSectionELF.cpp b/llvm/lib/MC/MCSectionELF.cpp
index 95fdf33522076e..1e1b5edb94d761 100644
--- a/llvm/lib/MC/MCSectionELF.cpp
+++ b/llvm/lib/MC/MCSectionELF.cpp
@@ -90,7 +90,9 @@ void MCSectionELF::printSwitchToSection(const MCAsmInfo &MAI, const Triple &T,
     OS << 'e';
   if (Flags & ELF::SHF_EXECINSTR)
     OS << 'x';
-  if (Flags & ELF::SHF_GROUP)
+  // TODO: Always print G after o to be clear that the 'G' argument is parsed
+  // after the 'o' argument.
+  if ((Flags & ELF::SHF_GROUP) && !(Flags & ELF::SHF_LINK_ORDER))
     OS << 'G';
   if (Flags & ELF::SHF_WRITE)
     OS << 'w';
@@ -102,6 +104,8 @@ void MCSectionELF::printSwitchToSection(const MCAsmInfo &MAI, const Triple &T,
     OS << 'T';
   if (Flags & ELF::SHF_LINK_ORDER)
     OS << 'o';
+  if ((Flags & ELF::SHF_GROUP) && (Flags & ELF::SHF_LINK_ORDER))
+    OS << 'G';
   if (Flags & ELF::SHF_GNU_RETAIN)
     OS << 'R';
 
@@ -183,13 +187,6 @@ void MCSectionELF::printSwitchToSection(const MCAsmInfo &MAI, const Triple &T,
     OS << "," << EntrySize;
   }
 
-  if (Flags & ELF::SHF_GROUP) {
-    OS << ",";
-    printName(OS, Group.getPointer()->getName());
-    if (isComdat())
-      OS << ",comdat";
-  }
-
   if (Flags & ELF::SHF_LINK_ORDER) {
     OS << ",";
     if (LinkedToSym)
@@ -198,6 +195,13 @@ void MCSectionELF::printSwitchToSection(const MCAsmInfo &MAI, const Triple &T,
       OS << '0';
   }
 
+  if (Flags & ELF::SHF_GROUP) {
+    OS << ",";
+    printName(OS, Group.getPointer()->getName());
+    if (isComdat())
+      OS << ",comdat";
+  }
+
   if (isUnique())
     OS << ",unique," << UniqueID;
 
diff --git a/llvm/test/CodeGen/AArch64/patchable-function-entry.ll b/llvm/test/CodeGen/AArch64/patchable-function-entry.ll
index 5750c1f601bdef..89a2e2bf4abbcf 100644
--- a/llvm/test/CodeGen/AArch64/patchable-function-entry.ll
+++ b/llvm/test/CodeGen/AArch64/patchable-function-entry.ll
@@ -48,7 +48,7 @@ define void @f3() "patchable-function-entry"="3" comdat {
 ; CHECK-NEXT: .Lfunc_begin3:
 ; CHECK-COUNT-3: nop
 ; CHECK-NEXT:  ret
-; CHECK:       .section __patchable_function_entries,"aGwo",@progbits,f3,comdat,f3{{$}}
+; CHECK:       .section __patchable_function_entries,"awoG",@progbits,f3,f3,comdat{{$}}
 ; CHECK-NEXT:  .p2align 3
 ; CHECK-NEXT:  .xword .Lfunc_begin3
   ret void
@@ -60,7 +60,7 @@ define void @f5() "patchable-function-entry"="5" comdat {
 ; CHECK-NEXT: .Lfunc_begin4:
 ; CHECK-COUNT-5: nop
 ; CHECK-NEXT:  sub sp, sp, #16
-; CHECK:       .section __patchable_function_entries,"aGwo",@progbits,f5,comdat,f5{{$}}
+; CHECK:       .section __patchable_function_entries,"awoG",@progbits,f5,f5,comdat{{$}}
 ; CHECK:       .p2align 3
 ; CHECK-NEXT:  .xword .Lfunc_begin4
   %frame = alloca i8, i32 16
diff --git a/llvm/test/CodeGen/LoongArch/patchable-function-entry.ll b/llvm/test/CodeGen/LoongArch/patchable-function-entry.ll
index aaa3fda1ae7769..2e390d1e2c33af 100644
--- a/llvm/test/CodeGen/LoongArch/patchable-function-entry.ll
+++ b/llvm/test/CodeGen/LoongArch/patchable-function-entry.ll
@@ -31,7 +31,7 @@ define void @f5() "patchable-function-entry"="5" comdat {
 ; CHECK-NEXT:    .Lfunc_begin2:
 ; CHECK-COUNT-5:   nop
 ; CHECK-NEXT:      ret
-; CHECK:         .section __patchable_function_entries,"aGwo",@progbits,f5,comdat,f5{{$}}
+; CHECK:         .section __patchable_function_entries,"awoG",@progbits,f5,f5,comdat{{$}}
 ; LA32:          .p2align 2
 ; LA32-NEXT:     .word .Lfunc_begin2
 ; LA64:          .p2align 3
diff --git a/llvm/test/CodeGen/Mips/xray-section-group.ll b/llvm/test/CodeGen/Mips/xray-section-group.ll
index 5a208217092dd7..e9cd045d4411e1 100644
--- a/llvm/test/CodeGen/Mips/xray-section-group.ll
+++ b/llvm/test/CodeGen/Mips/xray-section-group.ll
@@ -24,7 +24,7 @@ $bar = comdat any
 define i32 @bar() nounwind noinline uwtable "function-instrument"="xray-always" comdat($bar) {
 ; CHECK: .section .text.bar,"axG",@progbits,bar,comdat
   ret i32 1
-; CHECK: .section xray_instr_map,"aGo",@progbits,bar,comdat,bar{{$}}
+; CHECK: .section xray_instr_map,"aoG",@progbits,bar,bar,comdat{{$}}
 }
 
 ; CHECK-OBJ: Section {
diff --git a/llvm/test/CodeGen/RISCV/patchable-function-entry.ll b/llvm/test/CodeGen/RISCV/patchable-function-entry.ll
index 2804fdfc1ac9c5..4eeb1bf3138580 100644
--- a/llvm/test/CodeGen/RISCV/patchable-function-entry.ll
+++ b/llvm/test/CodeGen/RISCV/patchable-function-entry.ll
@@ -37,7 +37,7 @@ define void @f5() "patchable-function-entry"="5" comdat {
 ; NORVC-NEXT:    jalr zero, 0(ra)
 ; RVC-COUNT-5:   c.nop
 ; RVC-NEXT:      c.jr ra
-; CHECK:       .section __patchable_function_entries,"aGwo",@progbits,f5,comdat,f5{{$}}
+; CHECK:       .section __patchable_function_entries,"awoG",@progbits,f5,f5,comdat{{$}}
 ; RV32:        .p2align 2
 ; RV32-NEXT:   .word .Lfunc_begin2
 ; RV64:        .p2align 3
diff --git a/llvm/test/CodeGen/X86/basic-block-sections-labels-functions-sections.ll b/llvm/test/CodeGen/X86/basic-block-sections-labels-functions-sections.ll
index b8217bbc00760f..3be3ab70f8a693 100644
--- a/llvm/test/CodeGen/X86/basic-block-sections-labels-functions-sections.ll
+++ b/llvm/test/CodeGen/X86/basic-block-sections-labels-functions-sections.ll
@@ -35,7 +35,7 @@ define linkonce_odr dso_local i32 @_Z4fooTIiET_v() comdat {
 ; CHECK:		.section .text._Z4fooTIiET_v,"axG",@progbits,_Z4fooTIiET_v,comdat
 ; CHECK-LABEL:	_Z4fooTIiET_v:
 ; CHECK-NEXT:	[[FOOCOMDAT_BEGIN:.Lfunc_begin[0-9]+]]:
-; CHECK:		.section .llvm_bb_addr_map,"Go",@llvm_bb_addr_map,_Z4fooTIiET_v,comdat,.text._Z4fooTIiET_v{{$}}
+; CHECK:		.section .llvm_bb_addr_map,"oG",@llvm_bb_addr_map,.text._Z4fooTIiET_v,_Z4fooTIiET_v,comdat{{$}}
 ; CHECK-NEXT:		.byte 2				# version
 ; CHECK-NEXT:		.byte 0				# feature
 ; CHECK-NEXT:		.quad [[FOOCOMDAT_BEGIN]]	# function address
diff --git a/llvm/test/CodeGen/X86/gcc_except_table-multi.ll b/llvm/test/CodeGen/X86/gcc_except_table-multi.ll
index 8da3ebfed2bdd7..1eb902ae90795d 100644
--- a/llvm/test/CodeGen/X86/gcc_except_table-multi.ll
+++ b/llvm/test/CodeGen/X86/gcc_except_table-multi.ll
@@ -17,7 +17,7 @@ define i32 @group() uwtable comdat personality ptr @__gxx_personality_v0 {
 ; CHECK:             .cfi_endproc
 ; NORMAL-NEXT:       .section .gcc_except_table.group,"aG",@progbits,group,comdat{{$}}
 ; SEP_BFD-NEXT:      .section .gcc_except_table.group,"aG",@progbits,group,comdat{{$}}
-; SEP-NEXT:          .section .gcc_except_table.group,"aGo",@progbits,group,comdat,group{{$}}
+; SEP-NEXT:          .section .gcc_except_table.group,"aoG",@progbits,group,group,comdat{{$}}
 ; SEP_NOUNIQUE-NEXT: .section .gcc_except_table,"aG",@progbits,group,comdat{{$}}
 ; NOUNIQUE-NEXT:     .section .gcc_except_table,"aG",@progbits,group,comdat{{$}}
 entry:
@@ -61,7 +61,7 @@ define i32 @zero() uwtable comdat personality ptr @__gxx_personality_v0 {
 ; CHECK:             .cfi_endproc
 ; NORMAL-NEXT:       .section .gcc_except_table.zero,"aG",@progbits,zero{{$}}
 ; SEP_BFD-NEXT:      .section .gcc_except_table.zero,"aG",@progbits,zero{{$}}
-; SEP-NEXT:          .section .gcc_except_table.zero,"aGo",@progbits,zero,zero{{$}}
+; SEP-NEXT:          .section .gcc_except_table.zero,"aoG",@progbits,zero,zero{{$}}
 ; SEP_NOUNIQUE-NEXT: .section .gcc_except_table,"aG",@progbits,zero{{$}}
 ; NOUNIQUE-NEXT:     .section .gcc_except_table,"aG",@progbits,zero{{$}}
 entry:
diff --git a/llvm/test/CodeGen/X86/patchable-function-entry.ll b/llvm/test/CodeGen/X86/patchable-function-entry.ll
index 124f5c57c74b5a..8c37f545108015 100644
--- a/llvm/test/CodeGen/X86/patchable-function-entry.ll
+++ b/llvm/test/CodeGen/X86/patchable-function-entry.ll
@@ -50,7 +50,7 @@ define void @f3() "patchable-function-entry"="3" comdat {
 ; 32-NEXT:     nop
 ; 64:          nopl (%rax)
 ; CHECK:       ret
-; CHECK:       .section __patchable_function_entries,"aGwo",@progbits,f3,comdat,f3{{$}}
+; CHECK:       .section __patchable_function_entries,"awoG",@progbits,f3,f3,comdat{{$}}
 ; 32:          .p2align 2
 ; 32-NEXT:     .long .Lfunc_begin3
 ; 64:          .p2align 3
@@ -66,7 +66,7 @@ define void @f5() "patchable-function-entry"="5" comdat {
 ; 32-NEXT:     nop
 ; 64:          nopl 8(%rax,%rax)
 ; CHECK-NEXT:  ret
-; CHECK:       .section __patchable_function_entries,"aGwo",@progbits,f5,comdat,f5{{$}}
+; CHECK:       .section __patchable_function_entries,"awoG",@progbits,f5,f5,comdat{{$}}
 ; 32:          .p2align 2
 ; 32-NEXT:     .long .Lfunc_begin4
 ; 64:          .p2align 3
diff --git a/llvm/test/CodeGen/X86/stack-size-section-function-sections.ll b/llvm/test/CodeGen/X86/stack-size-section-function-sections.ll
index 92f312bd1185ce..b9606c081a90e0 100644
--- a/llvm/test/CodeGen/X86/stack-size-section-function-sections.ll
+++ b/llvm/test/CodeGen/X86/stack-size-section-function-sections.ll
@@ -15,9 +15,9 @@
 
 ; Check we add .stack_size section to a COMDAT group with the corresponding .text section if such a COMDAT exists.
 ; UNIQ:   .section        .text._Z4fooTIiET_v,"axG",@progbits,_Z4fooTIiET_v,comdat{{$}}
-; UNIQ:   .section        .stack_sizes,"Go",@progbits,_Z4fooTIiET_v,comdat,.text._Z4fooTIiET_v{{$}}
+; UNIQ:   .section        .stack_sizes,"oG",@progbits,.text._Z4fooTIiET_v,_Z4fooTIiET_v,comdat{{$}}
 ; NOUNIQ: .section        .text,"axG",@progbits,_Z4fooTIiET_v,comdat,unique,3
-; NOUNIQ: .section        .stack_sizes,"Go",@progbits,_Z4fooTIiET_v,comdat,.text,unique,3
+; NOUNIQ: .section        .stack_sizes,"oG",@progbits,.text,_Z4fooTIiET_v,comdat,unique,3
 
 $_Z4fooTIiET_v = comdat any
 
diff --git a/llvm/test/CodeGen/X86/stack-size-section.ll b/llvm/test/CodeGen/X86/stack-size-section.ll
index 3652ee845a7f34..866acbe140147e 100644
--- a/llvm/test/CodeGen/X86/stack-size-section.ll
+++ b/llvm/test/CodeGen/X86/stack-size-section.ll
@@ -29,7 +29,7 @@ define void @func2() #0 {
 
 ; Check that we still put .stack_sizes into the corresponding COMDAT group if any.
 ; CHECK: .section .text._Z4fooTIiET_v,"axG",@progbits,_Z4fooTIiET_v,comdat
-; GROUPS: .section .stack_sizes,"Go",@progbits,_Z4fooTIiET_v,comdat,.text._Z4fooTIiET_v{{$}}
+; GROUPS: .section .stack_sizes,"oG",@progbits,.text._Z4fooTIiET_v,_Z4fooTIiET_v,comdat{{$}}
 ; NOGROUPS: .section .stack_sizes,"",@progbits
 $_Z4fooTIiET_v = comdat any
 define linkonce_odr dso_local i32 @_Z4fooTIiET_v() comdat {
diff --git a/llvm/test/CodeGen/X86/xray-section-group.ll b/llvm/test/CodeGen/X86/xray-section-group.ll
index c05520adf89972..1f2855b089d22e 100644
--- a/llvm/test/CodeGen/X86/xray-section-group.ll
+++ b/llvm/test/CodeGen/X86/xray-section-group.ll
@@ -12,7 +12,7 @@ $bar = comdat any
 define i32 @bar() nounwind noinline uwtable "function-instrument"="xray-always" comdat($bar) {
 ; CHECK: .section .text.bar,"axG",@progbits,bar,comdat
   ret i32 1
-; CHECK: .section xray_instr_map,"aGo",@progbits,bar,comdat,bar{{$}}
+; CHECK: .section xray_instr_map,"aoG",@progbits,bar,bar,comdat{{$}}
 }
 
 ; CHECK-OBJ:      section xray_instr_map:
diff --git a/llvm/test/MC/ELF/section-combine.s b/llvm/test/MC/ELF/section-combine.s
index b68eaa3a05afaa..a6da8581a056e0 100644
--- a/llvm/test/MC/ELF/section-combine.s
+++ b/llvm/test/MC/ELF/section-combine.s
@@ -39,10 +39,10 @@ bar:
 .section .foo,"o",@progbits,bar,unique,1
 .byte 5
 
-.section .foo,"Go",@progbits,comdat0,comdat,bar,unique,1
+.section .foo,"Go",@progbits,bar,comdat0,comdat,unique,1
 .byte 6
 
-.section .foo,"Go",@progbits,comdat1,comdat,bar,unique,1
+.section .foo,"Go",@progbits,bar,comdat1,comdat,unique,1
 .byte 7
-.section .foo,"Go",@progbits,comdat1,comdat,bar,unique,1
+.section .foo,"oG",@progbits,bar,comdat1,comdat,unique,1
 .byte 8

@llvmbot
Copy link
Member

llvmbot commented Jan 9, 2024

@llvm/pr-subscribers-backend-x86

Author: Fangrui Song (MaskRay)

Changes

When both SHF_LINK_ORDER | SHF_GROUP flags are set, GNU assembler from
2.35 onwards (https://sourceware.org/PR25381
https://sourceware.org/binutils/docs/as/Section.html) parses the
SHF_LINK_ORDER argument before section group name, different from us.

This is unfortunate, but does not matter because the .section flag o
is a niche feature only used by compiler instrumentations, not adopted
by hand-written assembly, and using both flags is extremely rare. Let's
just match GNU assembler. While here, print 'G' after 'o' to be clear
that the 'G' argument is parsed after the 'o' argument. To make the diff
smaller, we don't print 'G' after 'w' in the absence of 'o' for now.

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

13 Files Affected:

  • (modified) llvm/lib/MC/MCParser/ELFAsmParser.cpp (+3-3)
  • (modified) llvm/lib/MC/MCSectionELF.cpp (+12-8)
  • (modified) llvm/test/CodeGen/AArch64/patchable-function-entry.ll (+2-2)
  • (modified) llvm/test/CodeGen/LoongArch/patchable-function-entry.ll (+1-1)
  • (modified) llvm/test/CodeGen/Mips/xray-section-group.ll (+1-1)
  • (modified) llvm/test/CodeGen/RISCV/patchable-function-entry.ll (+1-1)
  • (modified) llvm/test/CodeGen/X86/basic-block-sections-labels-functions-sections.ll (+1-1)
  • (modified) llvm/test/CodeGen/X86/gcc_except_table-multi.ll (+2-2)
  • (modified) llvm/test/CodeGen/X86/patchable-function-entry.ll (+2-2)
  • (modified) llvm/test/CodeGen/X86/stack-size-section-function-sections.ll (+2-2)
  • (modified) llvm/test/CodeGen/X86/stack-size-section.ll (+1-1)
  • (modified) llvm/test/CodeGen/X86/xray-section-group.ll (+1-1)
  • (modified) llvm/test/MC/ELF/section-combine.s (+3-3)
diff --git a/llvm/lib/MC/MCParser/ELFAsmParser.cpp b/llvm/lib/MC/MCParser/ELFAsmParser.cpp
index 93e1d2f44b8c56..d4c4bcb8564889 100644
--- a/llvm/lib/MC/MCParser/ELFAsmParser.cpp
+++ b/llvm/lib/MC/MCParser/ELFAsmParser.cpp
@@ -616,12 +616,12 @@ bool ELFAsmParser::ParseSectionArguments(bool IsPush, SMLoc loc) {
     if (Mergeable)
       if (parseMergeSize(Size))
         return true;
-    if (Group)
-      if (parseGroup(GroupName, IsComdat))
-        return true;
     if (Flags & ELF::SHF_LINK_ORDER)
       if (parseLinkedToSym(LinkedToSym))
         return true;
+    if (Group)
+      if (parseGroup(GroupName, IsComdat))
+        return true;
     if (maybeParseUniqueID(UniqueID))
       return true;
   }
diff --git a/llvm/lib/MC/MCSectionELF.cpp b/llvm/lib/MC/MCSectionELF.cpp
index 95fdf33522076e..1e1b5edb94d761 100644
--- a/llvm/lib/MC/MCSectionELF.cpp
+++ b/llvm/lib/MC/MCSectionELF.cpp
@@ -90,7 +90,9 @@ void MCSectionELF::printSwitchToSection(const MCAsmInfo &MAI, const Triple &T,
     OS << 'e';
   if (Flags & ELF::SHF_EXECINSTR)
     OS << 'x';
-  if (Flags & ELF::SHF_GROUP)
+  // TODO: Always print G after o to be clear that the 'G' argument is parsed
+  // after the 'o' argument.
+  if ((Flags & ELF::SHF_GROUP) && !(Flags & ELF::SHF_LINK_ORDER))
     OS << 'G';
   if (Flags & ELF::SHF_WRITE)
     OS << 'w';
@@ -102,6 +104,8 @@ void MCSectionELF::printSwitchToSection(const MCAsmInfo &MAI, const Triple &T,
     OS << 'T';
   if (Flags & ELF::SHF_LINK_ORDER)
     OS << 'o';
+  if ((Flags & ELF::SHF_GROUP) && (Flags & ELF::SHF_LINK_ORDER))
+    OS << 'G';
   if (Flags & ELF::SHF_GNU_RETAIN)
     OS << 'R';
 
@@ -183,13 +187,6 @@ void MCSectionELF::printSwitchToSection(const MCAsmInfo &MAI, const Triple &T,
     OS << "," << EntrySize;
   }
 
-  if (Flags & ELF::SHF_GROUP) {
-    OS << ",";
-    printName(OS, Group.getPointer()->getName());
-    if (isComdat())
-      OS << ",comdat";
-  }
-
   if (Flags & ELF::SHF_LINK_ORDER) {
     OS << ",";
     if (LinkedToSym)
@@ -198,6 +195,13 @@ void MCSectionELF::printSwitchToSection(const MCAsmInfo &MAI, const Triple &T,
       OS << '0';
   }
 
+  if (Flags & ELF::SHF_GROUP) {
+    OS << ",";
+    printName(OS, Group.getPointer()->getName());
+    if (isComdat())
+      OS << ",comdat";
+  }
+
   if (isUnique())
     OS << ",unique," << UniqueID;
 
diff --git a/llvm/test/CodeGen/AArch64/patchable-function-entry.ll b/llvm/test/CodeGen/AArch64/patchable-function-entry.ll
index 5750c1f601bdef..89a2e2bf4abbcf 100644
--- a/llvm/test/CodeGen/AArch64/patchable-function-entry.ll
+++ b/llvm/test/CodeGen/AArch64/patchable-function-entry.ll
@@ -48,7 +48,7 @@ define void @f3() "patchable-function-entry"="3" comdat {
 ; CHECK-NEXT: .Lfunc_begin3:
 ; CHECK-COUNT-3: nop
 ; CHECK-NEXT:  ret
-; CHECK:       .section __patchable_function_entries,"aGwo",@progbits,f3,comdat,f3{{$}}
+; CHECK:       .section __patchable_function_entries,"awoG",@progbits,f3,f3,comdat{{$}}
 ; CHECK-NEXT:  .p2align 3
 ; CHECK-NEXT:  .xword .Lfunc_begin3
   ret void
@@ -60,7 +60,7 @@ define void @f5() "patchable-function-entry"="5" comdat {
 ; CHECK-NEXT: .Lfunc_begin4:
 ; CHECK-COUNT-5: nop
 ; CHECK-NEXT:  sub sp, sp, #16
-; CHECK:       .section __patchable_function_entries,"aGwo",@progbits,f5,comdat,f5{{$}}
+; CHECK:       .section __patchable_function_entries,"awoG",@progbits,f5,f5,comdat{{$}}
 ; CHECK:       .p2align 3
 ; CHECK-NEXT:  .xword .Lfunc_begin4
   %frame = alloca i8, i32 16
diff --git a/llvm/test/CodeGen/LoongArch/patchable-function-entry.ll b/llvm/test/CodeGen/LoongArch/patchable-function-entry.ll
index aaa3fda1ae7769..2e390d1e2c33af 100644
--- a/llvm/test/CodeGen/LoongArch/patchable-function-entry.ll
+++ b/llvm/test/CodeGen/LoongArch/patchable-function-entry.ll
@@ -31,7 +31,7 @@ define void @f5() "patchable-function-entry"="5" comdat {
 ; CHECK-NEXT:    .Lfunc_begin2:
 ; CHECK-COUNT-5:   nop
 ; CHECK-NEXT:      ret
-; CHECK:         .section __patchable_function_entries,"aGwo",@progbits,f5,comdat,f5{{$}}
+; CHECK:         .section __patchable_function_entries,"awoG",@progbits,f5,f5,comdat{{$}}
 ; LA32:          .p2align 2
 ; LA32-NEXT:     .word .Lfunc_begin2
 ; LA64:          .p2align 3
diff --git a/llvm/test/CodeGen/Mips/xray-section-group.ll b/llvm/test/CodeGen/Mips/xray-section-group.ll
index 5a208217092dd7..e9cd045d4411e1 100644
--- a/llvm/test/CodeGen/Mips/xray-section-group.ll
+++ b/llvm/test/CodeGen/Mips/xray-section-group.ll
@@ -24,7 +24,7 @@ $bar = comdat any
 define i32 @bar() nounwind noinline uwtable "function-instrument"="xray-always" comdat($bar) {
 ; CHECK: .section .text.bar,"axG",@progbits,bar,comdat
   ret i32 1
-; CHECK: .section xray_instr_map,"aGo",@progbits,bar,comdat,bar{{$}}
+; CHECK: .section xray_instr_map,"aoG",@progbits,bar,bar,comdat{{$}}
 }
 
 ; CHECK-OBJ: Section {
diff --git a/llvm/test/CodeGen/RISCV/patchable-function-entry.ll b/llvm/test/CodeGen/RISCV/patchable-function-entry.ll
index 2804fdfc1ac9c5..4eeb1bf3138580 100644
--- a/llvm/test/CodeGen/RISCV/patchable-function-entry.ll
+++ b/llvm/test/CodeGen/RISCV/patchable-function-entry.ll
@@ -37,7 +37,7 @@ define void @f5() "patchable-function-entry"="5" comdat {
 ; NORVC-NEXT:    jalr zero, 0(ra)
 ; RVC-COUNT-5:   c.nop
 ; RVC-NEXT:      c.jr ra
-; CHECK:       .section __patchable_function_entries,"aGwo",@progbits,f5,comdat,f5{{$}}
+; CHECK:       .section __patchable_function_entries,"awoG",@progbits,f5,f5,comdat{{$}}
 ; RV32:        .p2align 2
 ; RV32-NEXT:   .word .Lfunc_begin2
 ; RV64:        .p2align 3
diff --git a/llvm/test/CodeGen/X86/basic-block-sections-labels-functions-sections.ll b/llvm/test/CodeGen/X86/basic-block-sections-labels-functions-sections.ll
index b8217bbc00760f..3be3ab70f8a693 100644
--- a/llvm/test/CodeGen/X86/basic-block-sections-labels-functions-sections.ll
+++ b/llvm/test/CodeGen/X86/basic-block-sections-labels-functions-sections.ll
@@ -35,7 +35,7 @@ define linkonce_odr dso_local i32 @_Z4fooTIiET_v() comdat {
 ; CHECK:		.section .text._Z4fooTIiET_v,"axG",@progbits,_Z4fooTIiET_v,comdat
 ; CHECK-LABEL:	_Z4fooTIiET_v:
 ; CHECK-NEXT:	[[FOOCOMDAT_BEGIN:.Lfunc_begin[0-9]+]]:
-; CHECK:		.section .llvm_bb_addr_map,"Go",@llvm_bb_addr_map,_Z4fooTIiET_v,comdat,.text._Z4fooTIiET_v{{$}}
+; CHECK:		.section .llvm_bb_addr_map,"oG",@llvm_bb_addr_map,.text._Z4fooTIiET_v,_Z4fooTIiET_v,comdat{{$}}
 ; CHECK-NEXT:		.byte 2				# version
 ; CHECK-NEXT:		.byte 0				# feature
 ; CHECK-NEXT:		.quad [[FOOCOMDAT_BEGIN]]	# function address
diff --git a/llvm/test/CodeGen/X86/gcc_except_table-multi.ll b/llvm/test/CodeGen/X86/gcc_except_table-multi.ll
index 8da3ebfed2bdd7..1eb902ae90795d 100644
--- a/llvm/test/CodeGen/X86/gcc_except_table-multi.ll
+++ b/llvm/test/CodeGen/X86/gcc_except_table-multi.ll
@@ -17,7 +17,7 @@ define i32 @group() uwtable comdat personality ptr @__gxx_personality_v0 {
 ; CHECK:             .cfi_endproc
 ; NORMAL-NEXT:       .section .gcc_except_table.group,"aG",@progbits,group,comdat{{$}}
 ; SEP_BFD-NEXT:      .section .gcc_except_table.group,"aG",@progbits,group,comdat{{$}}
-; SEP-NEXT:          .section .gcc_except_table.group,"aGo",@progbits,group,comdat,group{{$}}
+; SEP-NEXT:          .section .gcc_except_table.group,"aoG",@progbits,group,group,comdat{{$}}
 ; SEP_NOUNIQUE-NEXT: .section .gcc_except_table,"aG",@progbits,group,comdat{{$}}
 ; NOUNIQUE-NEXT:     .section .gcc_except_table,"aG",@progbits,group,comdat{{$}}
 entry:
@@ -61,7 +61,7 @@ define i32 @zero() uwtable comdat personality ptr @__gxx_personality_v0 {
 ; CHECK:             .cfi_endproc
 ; NORMAL-NEXT:       .section .gcc_except_table.zero,"aG",@progbits,zero{{$}}
 ; SEP_BFD-NEXT:      .section .gcc_except_table.zero,"aG",@progbits,zero{{$}}
-; SEP-NEXT:          .section .gcc_except_table.zero,"aGo",@progbits,zero,zero{{$}}
+; SEP-NEXT:          .section .gcc_except_table.zero,"aoG",@progbits,zero,zero{{$}}
 ; SEP_NOUNIQUE-NEXT: .section .gcc_except_table,"aG",@progbits,zero{{$}}
 ; NOUNIQUE-NEXT:     .section .gcc_except_table,"aG",@progbits,zero{{$}}
 entry:
diff --git a/llvm/test/CodeGen/X86/patchable-function-entry.ll b/llvm/test/CodeGen/X86/patchable-function-entry.ll
index 124f5c57c74b5a..8c37f545108015 100644
--- a/llvm/test/CodeGen/X86/patchable-function-entry.ll
+++ b/llvm/test/CodeGen/X86/patchable-function-entry.ll
@@ -50,7 +50,7 @@ define void @f3() "patchable-function-entry"="3" comdat {
 ; 32-NEXT:     nop
 ; 64:          nopl (%rax)
 ; CHECK:       ret
-; CHECK:       .section __patchable_function_entries,"aGwo",@progbits,f3,comdat,f3{{$}}
+; CHECK:       .section __patchable_function_entries,"awoG",@progbits,f3,f3,comdat{{$}}
 ; 32:          .p2align 2
 ; 32-NEXT:     .long .Lfunc_begin3
 ; 64:          .p2align 3
@@ -66,7 +66,7 @@ define void @f5() "patchable-function-entry"="5" comdat {
 ; 32-NEXT:     nop
 ; 64:          nopl 8(%rax,%rax)
 ; CHECK-NEXT:  ret
-; CHECK:       .section __patchable_function_entries,"aGwo",@progbits,f5,comdat,f5{{$}}
+; CHECK:       .section __patchable_function_entries,"awoG",@progbits,f5,f5,comdat{{$}}
 ; 32:          .p2align 2
 ; 32-NEXT:     .long .Lfunc_begin4
 ; 64:          .p2align 3
diff --git a/llvm/test/CodeGen/X86/stack-size-section-function-sections.ll b/llvm/test/CodeGen/X86/stack-size-section-function-sections.ll
index 92f312bd1185ce..b9606c081a90e0 100644
--- a/llvm/test/CodeGen/X86/stack-size-section-function-sections.ll
+++ b/llvm/test/CodeGen/X86/stack-size-section-function-sections.ll
@@ -15,9 +15,9 @@
 
 ; Check we add .stack_size section to a COMDAT group with the corresponding .text section if such a COMDAT exists.
 ; UNIQ:   .section        .text._Z4fooTIiET_v,"axG",@progbits,_Z4fooTIiET_v,comdat{{$}}
-; UNIQ:   .section        .stack_sizes,"Go",@progbits,_Z4fooTIiET_v,comdat,.text._Z4fooTIiET_v{{$}}
+; UNIQ:   .section        .stack_sizes,"oG",@progbits,.text._Z4fooTIiET_v,_Z4fooTIiET_v,comdat{{$}}
 ; NOUNIQ: .section        .text,"axG",@progbits,_Z4fooTIiET_v,comdat,unique,3
-; NOUNIQ: .section        .stack_sizes,"Go",@progbits,_Z4fooTIiET_v,comdat,.text,unique,3
+; NOUNIQ: .section        .stack_sizes,"oG",@progbits,.text,_Z4fooTIiET_v,comdat,unique,3
 
 $_Z4fooTIiET_v = comdat any
 
diff --git a/llvm/test/CodeGen/X86/stack-size-section.ll b/llvm/test/CodeGen/X86/stack-size-section.ll
index 3652ee845a7f34..866acbe140147e 100644
--- a/llvm/test/CodeGen/X86/stack-size-section.ll
+++ b/llvm/test/CodeGen/X86/stack-size-section.ll
@@ -29,7 +29,7 @@ define void @func2() #0 {
 
 ; Check that we still put .stack_sizes into the corresponding COMDAT group if any.
 ; CHECK: .section .text._Z4fooTIiET_v,"axG",@progbits,_Z4fooTIiET_v,comdat
-; GROUPS: .section .stack_sizes,"Go",@progbits,_Z4fooTIiET_v,comdat,.text._Z4fooTIiET_v{{$}}
+; GROUPS: .section .stack_sizes,"oG",@progbits,.text._Z4fooTIiET_v,_Z4fooTIiET_v,comdat{{$}}
 ; NOGROUPS: .section .stack_sizes,"",@progbits
 $_Z4fooTIiET_v = comdat any
 define linkonce_odr dso_local i32 @_Z4fooTIiET_v() comdat {
diff --git a/llvm/test/CodeGen/X86/xray-section-group.ll b/llvm/test/CodeGen/X86/xray-section-group.ll
index c05520adf89972..1f2855b089d22e 100644
--- a/llvm/test/CodeGen/X86/xray-section-group.ll
+++ b/llvm/test/CodeGen/X86/xray-section-group.ll
@@ -12,7 +12,7 @@ $bar = comdat any
 define i32 @bar() nounwind noinline uwtable "function-instrument"="xray-always" comdat($bar) {
 ; CHECK: .section .text.bar,"axG",@progbits,bar,comdat
   ret i32 1
-; CHECK: .section xray_instr_map,"aGo",@progbits,bar,comdat,bar{{$}}
+; CHECK: .section xray_instr_map,"aoG",@progbits,bar,bar,comdat{{$}}
 }
 
 ; CHECK-OBJ:      section xray_instr_map:
diff --git a/llvm/test/MC/ELF/section-combine.s b/llvm/test/MC/ELF/section-combine.s
index b68eaa3a05afaa..a6da8581a056e0 100644
--- a/llvm/test/MC/ELF/section-combine.s
+++ b/llvm/test/MC/ELF/section-combine.s
@@ -39,10 +39,10 @@ bar:
 .section .foo,"o",@progbits,bar,unique,1
 .byte 5
 
-.section .foo,"Go",@progbits,comdat0,comdat,bar,unique,1
+.section .foo,"Go",@progbits,bar,comdat0,comdat,unique,1
 .byte 6
 
-.section .foo,"Go",@progbits,comdat1,comdat,bar,unique,1
+.section .foo,"Go",@progbits,bar,comdat1,comdat,unique,1
 .byte 7
-.section .foo,"Go",@progbits,comdat1,comdat,bar,unique,1
+.section .foo,"oG",@progbits,bar,comdat1,comdat,unique,1
 .byte 8

@MaskRay
[MC] Parse SHF_LINK_ORDER argument before section group name
43e4b1e 
When both SHF_LINK_ORDER | SHF_GROUP flags are set, GNU assembler from
2.35 onwards (https://sourceware.org/PR25381
https://sourceware.org/binutils/docs/as/Section.html) parses the
SHF_LINK_ORDER argument before section group name, different from us.

This is unfortunate, but does not matter because the `.section` flag `o`
is a niche feature only used by compiler instrumentations, not adopted
by hand-written assembly, and using both flags is extremely rare. Let's
just match GNU assembler. There is another benefit: we now support
zero-flag section group with the SHF_LINK_ORDER flag, while previously
there isn't a syntax.

While here, print 'G' after 'o' to be clear that the 'G' argument is
parsed after the 'o' argument. To make the diff smaller, we don't print
'G' after 'w' in the absence of 'o' for now.
@MaskRay MaskRay force-pushed the mc-linkorder-group branch from 7893a7a to 43e4b1e Compare January 9, 2024 04:11
petrhosek
petrhosek reviewed Jan 9, 2024
View reviewed changes
llvm/lib/MC/MCSectionELF.cpp
@@ -90,7 +90,9 @@ void MCSectionELF::printSwitchToSection(const MCAsmInfo &MAI, const Triple &T,
OS << 'e';
if (Flags & ELF::SHF_EXECINSTR)
OS << 'x';
if (Flags & ELF::SHF_GROUP)
// TODO: Always print G after o to be clear that the 'G' argument is parsed
Copy link
Member

@petrhosek petrhosek Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can you file an issue and reference it here so we don't loose track of this TODO?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the quick review. I have just created #77422 to clean up this and update some tests (not that many), but I want to keep them separate from this change.

@MaskRay MaskRay merged commit 7620f03 into llvm:main Jan 9, 2024
@MaskRay MaskRay deleted the mc-linkorder-group branch January 9, 2024 18:42
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
@MaskRay @justinfargnoli
[MC] Parse SHF_LINK_ORDER argument before section group name (llvm#77407
2865067 
)

When both SHF_LINK_ORDER | SHF_GROUP flags are set, GNU assembler from
2.35 onwards (https://sourceware.org/PR25381
https://sourceware.org/binutils/docs/as/Section.html) parses the
SHF_LINK_ORDER argument before section group name, different from us.

This is unfortunate, but does not matter because the `.section` flag `o`
is a niche feature only used by compiler instrumentations, not adopted
by hand-written assembly, and using both flags is extremely rare. Let's
just match GNU assembler. There is another benefit: we now support
zero-flag section group with the SHF_LINK_ORDER flag, while previously
there isn't a syntax.

While here, print 'G' after 'o' to be clear that the 'G' argument is
parsed after the 'o' argument. To make the diff smaller, we don't print
'G' after 'w' in the absence of 'o' for now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrhosek petrhosek petrhosek approved these changes

@bd1976bris bd1976bris Awaiting requested review from bd1976bris

Assignees
No one assigned
Labels
backend:AArch64 backend:loongarch backend:X86 mc Machine (object) code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MaskRay @llvmbot @petrhosek