Align stubs for external and common global variables to pointer size.

[rahulchaudhry]

Summary

This patch fixes PR36885: clang++ generates unaligned stub symbol
holding a pointer.

[mgrang]

Can you please add a unit test?

[rahulchaudhry]

In D44848#1047203, @mgrang wrote:

Can you please add a unit test?

There is a minimized testcase in the bug report: https://bugs.llvm.org/show_bug.cgi?id=36885
I'm not sure which repo the test should be added to (llvm or clang?).
Should the test be generating asm or ll file and check for alignment directive on the stub?
It'll be useful if you can point me to an existing test that does something similar for regular (non-stub) symbols.
Any guidance will be appreciated. This is my first llvm change.

[mgrang]

You can add the test in llvm as an ll file. You can generate the ll as:

clang++ a.cpp -emit-llvm -S -o a.ll

Take a look at llvm/test/Object/ARM/symbol-addr.ll as an example to get started.
See also:
https://llvm.org/docs/CommandGuide/llc.html
https://llvm.org/docs/TestingGuide.html

[rahulchaudhry]

Added a unit test.

[rahulchaudhry]

In D44848#1047258, @mgrang wrote:

You can add the test in llvm as an ll file.

Done. Please take another look.

[mgrang]

test/Object/X86/align-stub.ll

1 ↗

(On Diff #140024)

Please follow 80 char limit.
See: https://llvm.org/docs/CodingStandards.html#source-code-width

Also -relocation-model=pic instead of --relocation-model=pic.

2 ↗

(On Diff #140024)

Remove the extraneous space after RUN:

[rahulchaudhry]

80-char line length limit, and use '-relocation-model=pic' instead of '--relocation-model=pic'.

[rahulchaudhry]

test/Object/X86/align-stub.ll

2 ↗

(On Diff #140024)

It's a continuation line now, so should the space stay, or would you like me to remove it?

[• espindola]

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1341

Do you need to align in every loop iteration?

test/Object/X86/align-stub.ll

2 ↗

(On Diff #140026)

Please don't use -filetype=obj. You can check for the align directive in the assembly.

[rahulchaudhry]

Moved EmitAlignment() out of the loop.

Remove '-filetype=obj' from test, checking for
alignment directive in assembly output instead.

[rahulchaudhry]

Updated the comment and CHECK directives to match each other.

[• espindola]

test/Object/X86/align-stub.ll

12 ↗

(On Diff #140318)

Code change LGTM, but can't you trim the test a bit? For example, why do you need two comdats to test a change on the exception table output?

[rahulchaudhry]

test/Object/X86/align-stub.ll

12 ↗

(On Diff #140318)

The test file was generated by running "clang++ -fPIE -emit-llvm -S " on the testcase in PR36885.
I removed a few lines of header and footer as it didn't matter.
I don't know llvm well enough to be comfortable editing this file by hand, or to write one from scratch.

Frankly, it's not the greatest idea that we're adding compiler generated output as tests, instead of the original source which is far more compact and readable.

[• espindola]

test/Object/X86/align-stub.ll

12 ↗

(On Diff #140318)

The fix is to reduce the .ll file :-)

Simply remove everything you can as long the new file still shows the results you want.

You can check https://llvm.org/docs/LangRef.html if you have any questions.

You should also use named temporaries. You can do that by passing the .ll to "opt -instnamer". This will give you a testcase that is much simpler to reduce.

[rahulchaudhry]

Minimized test case.

[rahulchaudhry]

test/Object/X86/align-stub.ll

12 ↗

(On Diff #140318)

Probably not what you expected, but I've minimized the testcase aggressively :)

[• espindola]

Awesome :-)

LGTM.

test/CodeGen/X86/catch.ll

8

Nit, the p2align is in another line, no? Could you use CHECK-NEXT?

[rahulchaudhry]

Use CHECK-NEXT to match p2align.

[rahulchaudhry]

Thanks for the review.
If everything looks good, can you land it as well.
This is my first change. I don't have write permissions to the repo.

test/CodeGen/X86/catch.ll

8

There were multiple .data directives in the original test, and I was trying to match the one preceding p2align.
In this case, there's only one .data, so simple CHECK/CHECK-NEXT sequence works fine.

[• espindola]

329030

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

if (TM.getTargetTriple().isOSBinFormatELF()) {
     MachineModuleInfoELF &MMIELF = MMI->getObjFileInfo<MachineModuleInfoELF>();
 
     // Output stubs for external and common global variables.
     MachineModuleInfoELF::SymbolListTy Stubs = MMIELF.GetGVStubList();
     if (!Stubs.empty()) {
       OutStreamer->SwitchSection(TLOF.getDataSection());
       const DataLayout &DL = M.getDataLayout();
 
+      EmitAlignment(Log2_32(DL.getPointerSize()));
       for (const auto &Stub : Stubs) {
         OutStreamer->EmitLabel(Stub.first);
         OutStreamer->EmitSymbolValue(Stub.second.getPointer(),
                                      DL.getPointerSize());
       }
     }
   }
 
   // Finalize debug and EH information.
   for (const HandlerInfo &HI : Handlers) {

test/CodeGen/X86/catch.ll

 ; RUN: llc < %s -mtriple=x86_64-pc-linux -relocation-model=pic | FileCheck %s
 
 ; PR18390
 ; We used to assert creating this label. The name itself is not critical. It
 ; just needs to be a unique local symbol.
-; CHECK:      .L.Lstr.DW.stub:
+; PR36885
+; The stub symbol should have pointer-size (8 byte) alignment.
+; CHECK:      .data{{[[:space:]]}} .p2align 3
+; CHECK-NEXT: .L.Lstr.DW.stub:
 ; CHECK-NEXT: .quad   .Lstr
 
 @str = private unnamed_addr constant [12 x i8] c"NSException\00"
 define void @f() personality i8* bitcast (void ()* @h to i8*) {
   invoke void @g()
           to label %invoke.cont unwind label %lpad
 invoke.cont:
   ret void
 lpad:
   %tmp14 = landingpad { i8*, i32 }
            catch i8* getelementptr inbounds ([12 x i8], [12 x i8]* @str, i64 0, i64 0)
   ret void
 }
 declare void @g()
 declare void @h()