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.

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

Clang: Putting a function in a special __TEXT,__foo section results in underalignment #90358

Closed
dougsonos opened this issue Apr 27, 2024 · 3 comments
Closed

Clang: Putting a function in a special __TEXT,__foo section results in underalignment #90358

dougsonos opened this issue Apr 27, 2024 · 3 comments
Labels
backend:AArch64

Comments

@dougsonos
Copy link
Contributor

To reproduce (on macOS):

// section_test.cpp
struct Class {
	__attribute__((section("__TEXT,__foo,regular,pure_instructions")))
	static int test(int x, int y)
	{
		return x + y;
	}
};

int main(int argc, const char*[])
{
	return Class::test(argc, argc);
}

// clang -c -o section_test.o section_test.cpp
// otool -l section_test.o

Output is:

Section
  sectname __foo
   segname __TEXT
      addr 0x0000000000000030
      size 0x0000000000000020
    offset 520
     align 2^1 (2)   <<<<<<<<<<< underaligned section

I would have hoped for an alignment of 4 because that is the (sub)target's getMinFunctionAlignment().

Analysis so far:

  • Section alignment comes from the most-aligned object in it - MCObjectStreamer
  • the llvm::Function has an alignment of 2 (comments say: for C++ ABI reasons)
  • the MachineFunction has an alignment of 4 (for arm64)
  • normally the MachineFunction's alignment is used, but because hasSection() is true for the Function, the alignment of 2 wins (logic in AsmPrinter::getGVAlignment)
  • the MCSection remains aligned to 2 because all of its functions are aligned to 2
  • the normal text section's alignment is 4 because the MachineFunctions are 4-byte-aligned and not overridden by AsmPrinter

This seems like a catch-22: the MachineFunction alignment should win, but is getting overridden by the section's alignment, yet the section's alignment comes from the llvm::Function, which doesn't know anything at all (would use 1 except for a C++ ABI reason making it 2).

Interestingly, a function that is not a C++ method declared inline in its class, if it has a section attribute, is aligned correctly.
@dougsonos
Copy link
Contributor Author

This fixes the immediate issue:

diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
index f85af2b73308..b028f5eeb57b 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -2967,9 +2968,15 @@ void AsmPrinter::emitAlignment(Align Alignment, const GlobalObject *GV,
 
   if (getCurrentSection()->getKind().isText()) {
     const MCSubtargetInfo *STI = nullptr;
-    if (this->MF)
-      STI = &getSubtargetInfo();
-    else
+    if (this->MF) {
+      const TargetSubtargetInfo *STI2 = &MF->getSubtarget();
+      const Align MinAlign = STI2->getTargetLowering()->getMinFunctionAlignment();
+      if (Alignment < MinAlign) {
+        Alignment = MinAlign;
+      }
+
+      STI = STI2;
+    } else
       STI = TM.getMCSubtargetInfo();
     OutStreamer->emitCodeAlignment(Alignment, STI, MaxBytesToEmit);
   } else

but I don't know if this is quite the right place for the fix.

@dougsonos dougsonos changed the title Putting a function in a special __TEXT,__foo section results in underalignment Clang: Putting a function in a special __TEXT,__foo section results in underalignment Apr 27, 2024
@dougsonos
Copy link
Contributor Author

dougsonos commented Apr 28, 2024 •
edited
Loading

Comparing the cases of 2 C++ methods, one with an explicit section attribute, one without:

  • AsmPrinter::emitAlignment receives an alignment of 4 in both cases (from MachineFunction).
  • it passes 4 to getGVAlignment
  • in both cases, the GV has an alignment of 2 (because C++ ABI)
  • but when there is no special section, the MachineFunction's alignment of 4 wins
  • when there is a specific section, the GV's alignment of 2 wins

Because (in getGVAlignment):

  // If the GVAlign is larger than NumBits, or if we are required to obey
  // NumBits because the GV has an assigned section, obey it.
  if (*GVAlign > Alignment || GV->hasSection())
    Alignment = *GVAlign;

Since the GV here is not necessarily code, it's probably not the right place for a fix. The place I outlined in the earlier seems correct, unless there is a legitimate case where text should be aligned to something less than the MachineFunction's alignment.

I considered not looking at getMinFunctionAlignment() since the incoming alignment here is already that of the MachineFunction -- but that can be increased from the target's minimum:

  // FIXME: Shouldn't use pref alignment if explicit alignment is set on F.
  // FIXME: Use Function::hasOptSize().
  if (!F.hasFnAttribute(Attribute::OptimizeForSize))
    Alignment = std::max(Alignment,
                         STI->getTargetLowering()->getPrefFunctionAlignment());

dougsonos pushed a commit to dougsonos/llvm-project that referenced this issue Apr 28, 2024
[llvm codegen] In AsmPrinter::emitAlignment, don't allow the possibly
527872f 
smaller alignment of the GlobalObject to reduce a function's alignment
below the target's minimum function alignment. (llvm#90358)
@dougsonos dougsonos mentioned this issue Apr 28, 2024
Closed
dougsonos pushed a commit to dougsonos/llvm-project that referenced this issue May 1, 2024
In AArch64's DataLayout, specify a minimum function alignment of 4.
4c312af 
This addresses an issue where the explicit alignment of 2 (for C++
ABI reasons) was being propagated to the back end and causing
under-aligned functions (in special sections). (llvm#90358)
@dougsonos dougsonos mentioned this issue May 1, 2024
Merged
@llvmbot
Copy link
Member

llvmbot commented May 6, 2024

@llvm/issue-subscribers-backend-aarch64

Author: Doug Wyatt (dougsonos)

To reproduce (on macOS): 
// section_test.cpp
struct Class {
	__attribute__((section("__TEXT,__foo,regular,pure_instructions")))
	static int test(int x, int y)
	{
		return x + y;
	}
};

int main(int argc, const char*[])
{
	return Class::test(argc, argc);
}

// clang -c -o section_test.o section_test.cpp
// otool -l section_test.o

Output is:

Section
  sectname __foo
   segname __TEXT
      addr 0x0000000000000030
      size 0x0000000000000020
    offset 520
     align 2^1 (2)   &lt;&lt;&lt;&lt;&lt;&lt;&lt;&lt;&lt;&lt;&lt; underaligned section

I would have hoped for an alignment of 4 because that is the (sub)target's getMinFunctionAlignment().

Analysis so far:

  • Section alignment comes from the most-aligned object in it - MCObjectStreamer
  • the llvm::Function has an alignment of 2 (comments say: for C++ ABI reasons)
  • the MachineFunction has an alignment of 4 (for arm64)
  • normally the MachineFunction's alignment is used, but because hasSection() is true for the Function, the alignment of 2 wins (logic in AsmPrinter::getGVAlignment)
  • the MCSection remains aligned to 2 because all of its functions are aligned to 2
  • the normal text section's alignment is 4 because the MachineFunctions are 4-byte-aligned and not overridden by AsmPrinter

This seems like a catch-22: the MachineFunction alignment should win, but is getting overridden by the section's alignment, yet the section's alignment comes from the llvm::Function, which doesn't know anything at all (would use 1 except for a C++ ABI reason making it 2).

Interestingly, a function that is not a C++ method declared inline in its class, if it has a section attribute, is aligned correctly.

AZero13 pushed a commit to AZero13/llvm-project that referenced this issue May 7, 2024
@dougsonos @AZero13
[clang backend] In AArch64's DataLayout, specify a minimum function a…
40b02c0 
…lignment of 4. (llvm#90702)

This addresses an issue where the explicit alignment of 2 (for C++ ABI
reasons) was being propagated to the back end and causing under-aligned
functions (in special sections).

This is an alternate approach suggested by @efriedma-quic in PR llvm#90415.

Fixes llvm#90358

(cherry picked from commit ddecada)
@AZero13 AZero13 mentioned this issue May 7, 2024
Closed
TNorthover pushed a commit to swiftlang/llvm-project that referenced this issue May 23, 2024
@dougsonos @TNorthover
[clang backend] In AArch64's DataLayout, specify a minimum function a…
188951d 
…lignment of 4. (llvm#90702)

This addresses an issue where the explicit alignment of 2 (for C++ ABI
reasons) was being propagated to the back end and causing under-aligned
functions (in special sections).

This is an alternate approach suggested by @efriedma-quic in PR llvm#90415.

Fixes llvm#90358
@TNorthover TNorthover mentioned this issue May 23, 2024
Open
@efriedma-quic efriedma-quic mentioned this issue Nov 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
backend:AArch64
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@EugeneZelenko @dougsonos @llvmbot