[PATCH] D46874: [MC] - Add .stack_size sections into groups and link them with .text

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In https://reviews.llvm.org/D46874#1102734, @grimar wrote:

> In https://reviews.llvm.org/D46874#1102694, @jhenderson wrote:
>
> > @grimar, what is the link-time performance of doing this on something with many functions?
>
>
> I did not yet measure. I can do the benchmark and return with the results.


Please do - we probably need to consider the impact on bfd and gold as well, although I personally am less concerned there.

> 
> 
>> Also, I think it would make more sense for the stack sizes section names to be derived from the "parent" section, a bit like relocation sections, so they'd be called something like `.stack_sizes.text._Z3foov` or possibly simply `.stack_sizes._Z3foov`. That way dumping tools can more easily dump the specific individual stack_sizes sections.
> 
> Maybe, but renaming is a subject for a different patch.

Okay, that's fine.

> Also, they linked with a `sh_link` field to their parents already. Dumping tools can use that.

Only when they want to dump an associated group. It doesn't allow easy dumping of just the single stack sizes section (e.g. via -j in objdump).

> And renaming to something like `.stack_sizes.XXX` would require linker side change to place them into the single output section I think,
>  as currently they are merged by name, just like other regular sections.

This surprises me. I thought that default grouping would match up to the first '.', like it does for e.g. .text or .data grouping (i.e. .text.foo and .text.bar end up in .text). Or are some sections special-cased for this?



================
Comment at: test/CodeGen/X86/stack-size-section.ll:16
 ; CHECK-NEXT: .Lfunc_begin1:
-; CHECK: .section .stack_sizes,"", at progbits
+; CHECK: .section .stack_sizes,"o", at progbits,.text,unique,1
 ; CHECK-NEXT: .quad .Lfunc_begin1
----------------
grimar wrote:
> jhenderson wrote:
> > I might be misunderstanding something here, but I don't think these should be uniqued, i.e. in my opinion, in this case, there should only be one stack_sizes section containing both entries, since there is only one text section.
> Probably that would be ideal. I am doing unification unconditionally. I think just everyone use -ffunction-sections. So I am not sure if it worth to
> add code to avoid doing unification for that particular case right now. (It just a few lines probably though)
> 
> Without doing unification asm produced would contain several declarations of `.section .stack_sizes,"", at progbits` which
> are combined into the single section in the object finally. (see original stack-size-section.ll)
> 
> Probably that would be ideal. I am doing unification unconditionally. I think just everyone use -ffunction-sections. So I am not sure if it worth to add code to avoid doing unification for that particular case right now. (It just a few lines probably though)
I'm not sure I'm willing to generalise that much. However, it may not matter if performance impact is minimal.

I don't think having multiple section declarations for the same section is a big deal. It's already the case for some other sections, at least to a small extent.


https://reviews.llvm.org/D46874