Use local symbols for creating .stack-size

[• espindola]

Summary

Right now .stack-size ends up having relocations that point to global symbols. This means that if the symbol from another file is selected at link time it will be referring to the wrong symbol.

This is a first step in fixing pr36717.

[bd1976llvm]

LGTM., as long as this doesn't cause the number of symbols to double up?

[• espindola]

In D44669#1048280, @bd1976llvm wrote:

LGTM., as long as this doesn't cause the number of symbols to double up?

No, in practice the assembler will convert the relocations to use section symbols instead.

[• espindola]

LGTMed

[• espindola]

r328581.

[bd1976llvm]

Hi Rafael. The SN-Linker has the following rule for metadata sections:

Relocations from "metadata sections" to global symbols are
treated as discarded if the chosen global was not from the
same object file. This makes sense as the metadata entry
for a function is only applicable to that one particular version.

I have mentioned this rule here:

https://groups.google.com/d/msg/generic-abi/A-1rbP8hFCA/z2xHWFQBCAAJ

It would be an obvious extension to your SHF_LINK_ORDER concept.
If we had this extension in ELF then we could remove this special case
in MC and remove all of these local symbols that are bloating the
assembler output.

What do you think?

[• espindola]

In D44669#1049038, @bd1976llvm wrote:

Hi Rafael. The SN-Linker has the following rule for metadata sections:

Relocations from "metadata sections" to global symbols are
treated as discarded if the chosen global was not from the
same object file. This makes sense as the metadata entry
for a function is only applicable to that one particular version.

I agree with Cary that it is a bad idea to break sections. What is needed is a liker like lld that is efficient at handling sections.

It would be an obvious extension to your SHF_LINK_ORDER concept.
If we had this extension in ELF then we could remove this special case
in MC and remove all of these local symbols that are bloating the
assembler output.

It is not an obvious extension. The obvious solution is to just use multiple sections. I am sorry I missed this when it went in, but we should never add an extension that requires a section that refers to multiple ones. I will fix llvm to use multiple sections for -ffunction-sections/comdat next.

And there is no local symbol pouting the output. The assembler will convert the reference to use section symbols.

[bd1976llvm]

Hi Rafael,

Thanks for the reply. I agree that multiple sections is conceptually a
better
approach. Let's keep the discussion to the gabi list as it has nothing to do
with your change here.

I will fix llvm to use multiple sections for -ffunction-sections/comdat

I agree with this and with the rest of your assessment in pr36717.

And there is no local symbol pouting the output. The assembler will

convert the reference to use section symbols.

I understand that and I thought your change was good. Apologies,
if "bloating" sounded bad I should have used "extra" instead. What I
meant was - wouldn't the assembly output have doubled up local/global
symbols? My idea is, could we find some way to not have to emit the
additional local symbols in the assembly file and hopefully to
remove the special case code in mc that is needed to emit them?

[• espindola]

I understand that and I thought your change was good. Apologies,
if "bloating" sounded bad I should have used "extra" instead. What I
meant was - wouldn't the assembly output have doubled up local/global
symbols? My idea is, could we find some way to not have to emit the
additional local symbols in the assembly file and hopefully to
remove the special case code in mc that is needed to emit them?

Why are extra symbols *in the assembly* a problem? We normally don't print assembly and using local symbols means that we have only one place that has to implement the logic for using section symbols in relocations. I quite like the rule: CodeGen and user written assembly files use local symbols everywhere and the assembler optimizes that when possible.

Having said that, if you really find the assembly verbosity problematic, getFunctionBegin could return a section symbol when the current function is at the start of a section.

[bd1976llvm]

Why are extra symbols *in the assembly* a problem? We normally don't print assembly

I suppose I like the assembly to look good :) I think that the first of the
following options reads nicely compared to the others:

    .globl _start
_start:
    ret
    .section ".stack_sizes","",@progbits
    .quad _start
    .uleb128 10

    .globl _start
_start:
.L_start:
    ret
    .section ".stack_sizes","",@progbits
    .quad .L_start
    .uleb128 10

    .globl first
first:
    ret
    .globl _start
_start:
    ret
    .section ".stack_sizes","",@progbits
    .quad .text+10
    .uleb128 10

Using local symbols means that we have only one place that has to implement the logic for using section symbols in relocations. I quite like the rule: CodeGen and user written assembly files use local symbols everywhere and the assembler optimizes that when possible.

This sounds like a sound design choice to me. (Although, I have to say that
as I'm more of a binary tools man than a compiler engineer, I actually hate
it when the assembler doesn't output exactly the object file I expect given
what I typed in.)

Having said that, if you really find the assembly verbosity problematic, getFunctionBegin could return a section symbol when the current function is at the start of a section.

This would certainly remove the extra local symbols. I like the purity of
your design principle with where only the assembler lowers to use section
symbols though. I'm on the fence here so unless someone else chimes in I
think leaving MC as-is is fine with me.

[joerg]

Given that some people like to post-process assembler files, using the section symbol directly is a bad idea. Adding the local symbols is fine.

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

void AsmPrinter::emitStackSizeSection(const MachineFunction &MF) {
   const MachineFrameInfo &FrameInfo = MF.getFrameInfo();
   // Don't emit functions with dynamic stack allocations.
   if (FrameInfo.hasVarSizedObjects())
     return;
 
   OutStreamer->PushSection();
   OutStreamer->SwitchSection(StackSizeSection);
 
-  const MCSymbol *FunctionSymbol = getSymbol(&MF.getFunction());
+  const MCSymbol *FunctionSymbol = getFunctionBegin();
   uint64_t StackSize = FrameInfo.getStackSize();
   OutStreamer->EmitSymbolValue(FunctionSymbol, TM.getProgramPointerSize());
   OutStreamer->EmitULEB128IntValue(StackSize);
 
   OutStreamer->PopSection();
 }
 
 static bool needFuncLabelsForEHOrDebugInfo(const MachineFunction &MF,

 void AsmPrinter::SetupMachineFunction(MachineFunction &MF) {
   this->MF = &MF;
   // Get the function symbol.
   CurrentFnSym = getSymbol(&MF.getFunction());
   CurrentFnSymForSize = CurrentFnSym;
   CurrentFnBegin = nullptr;
   CurExceptionSym = nullptr;
   bool NeedsLocalForSize = MAI->needsLocalForSize();
-  if (needFuncLabelsForEHOrDebugInfo(MF, MMI) || NeedsLocalForSize) {
+  if (needFuncLabelsForEHOrDebugInfo(MF, MMI) || NeedsLocalForSize ||
+      MF.getTarget().Options.EmitStackSizeSection) {
     CurrentFnBegin = createTempSymbol("func_begin");
     if (NeedsLocalForSize)
       CurrentFnSymForSize = CurrentFnBegin;
   }
 
   ORE = &getAnalysis<MachineOptimizationRemarkEmitterPass>().getORE();
   LI = &getAnalysis<MachineLoopInfo>();
 

test/CodeGen/ARM/stack-size-section.ll

 ; RUN: llc < %s -mtriple=armv7-linux -stack-size-section | FileCheck %s
 
 ; CHECK-LABEL: func1:
+; CHECK-NEXT: .Lfunc_begin0:
 ; CHECK: .section .stack_sizes,"",%progbits
-; CHECK-NEXT: .long func1
+; CHECK-NEXT: .long .Lfunc_begin0
 ; CHECK-NEXT: .byte 8
 define void @func1(i32, i32) #0 {
   alloca i32, align 4
   alloca i32, align 4
   ret void
 }
 
 ; CHECK-LABEL: func2:
+; CHECK-NEXT: .Lfunc_begin1:
 ; CHECK: .section .stack_sizes,"",%progbits
-; CHECK-NEXT: .long func2
+; CHECK-NEXT: .long .Lfunc_begin1
 ; CHECK-NEXT: .byte 16
 define void @func2() #0 {
   alloca i32, align 4
   call void @func1(i32 1, i32 2)
   ret void
 }
 
 ; CHECK-LABEL: dynalloc:
 ; CHECK-NOT: .section .stack_sizes
 define void @dynalloc(i32 %N) #0 {
   alloca i32, i32 %N
   ret void
 }
 
 attributes #0 = { "no-frame-pointer-elim"="true" }

test/CodeGen/SystemZ/stack-size-section.ll

 ; RUN: llc < %s -mtriple=s390x-linux-gnu -stack-size-section | FileCheck %s
 
 ; CHECK-LABEL: func1:
+; CHECK-NEXT: .Lfunc_begin0:
 ; CHECK: .section .stack_sizes,"",@progbits
-; CHECK-NEXT: .quad func1
+; CHECK-NEXT: .quad .Lfunc_begin0
 ; CHECK-NEXT: .byte 0
 define void @func1(i32, i32) #0 {
   ret void
 }
 
 ; CHECK-LABEL: func2:
+; CHECK-NEXT: .Lfunc_begin1:
 ; CHECK: .section .stack_sizes,"",@progbits
-; CHECK-NEXT: .quad func2
+; CHECK-NEXT: .quad .Lfunc_begin1
 ; CHECK-NEXT: .ascii  "\250\001"
 define void @func2(i32, i32) #0 {
   alloca i32, align 4
   alloca i32, align 4
   ret void
 }
 
 ; CHECK-LABEL: func3:
+; CHECK-NEXT: .Lfunc_begin2:
 ; CHECK: .section .stack_sizes,"",@progbits
-; CHECK-NEXT: .quad func3
+; CHECK-NEXT: .quad .Lfunc_begin2
 ; CHECK-NEXT: .ascii  "\250\001"
 define void @func3() #0 {
   alloca i32, align 4
   call void @func1(i32 1, i32 2)
   ret void
 }
 
 ; CHECK-LABEL: dynalloc:
 ; CHECK-NOT: .section .stack_sizes
 define void @dynalloc(i32 %N) #0 {
   alloca i32, i32 %N
   ret void
 }
 
 attributes #0 = { "no-frame-pointer-elim"="true" }

test/CodeGen/X86/stack-size-section.ll

 ; RUN: llc < %s -mtriple=x86_64-linux -stack-size-section | FileCheck %s
 
 ; CHECK-LABEL: func1:
+; CHECK-NEXT: .Lfunc_begin0:
 ; CHECK: .section .stack_sizes,"",@progbits
-; CHECK-NEXT: .quad func1
+; CHECK-NEXT: .quad .Lfunc_begin0
 ; CHECK-NEXT: .byte 8
 define void @func1(i32, i32) #0 {
   alloca i32, align 4
   alloca i32, align 4
   ret void
 }
 
 ; CHECK-LABEL: func2:
+; CHECK-NEXT: .Lfunc_begin1:
 ; CHECK: .section .stack_sizes,"",@progbits
-; CHECK-NEXT: .quad func2
+; CHECK-NEXT: .quad .Lfunc_begin1
 ; CHECK-NEXT: .byte 24
 define void @func2() #0 {
   alloca i32, align 4
   call void @func1(i32 1, i32 2)
   ret void
 }
 
 ; CHECK-LABEL: dynalloc:
 ; CHECK-NOT: .section .stack_sizes
 define void @dynalloc(i32 %N) #0 {
   alloca i32, i32 %N
   ret void
 }
 
 attributes #0 = { "no-frame-pointer-elim"="true" }