Page MenuHomePhabricator

[MC][ELF] Prevent globals with an explicit section from being mergeable
Needs ReviewPublic

Authored by bd1976llvm on Sep 26 2019, 1:07 PM.

Diff Detail

Event Timeline

bd1976llvm created this revision.Sep 26 2019, 1:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2019, 1:07 PM

This feels like it could cause a pretty serious regression. This essentially disables global merging with -fdata-sections, which I know at least one linker relies upon for code size.

It'd be good to get comments from the ARM folk, I've also added Sjoerd who I know has background in this.

Hello James! Thanks for informing us. If this does what you say, it essentially disables global merging with -fdata-sections, then indeed we would get a little bit unhappy about that.
I could run some numbers with this patch, but I can guess what the outcome will be..... I haven't looked at this patch or the PR, but does this need to be default behaviour, or can this e.g. be done under an option?

This feels like it could cause a pretty serious regression. This essentially disables global merging with -fdata-sections, which I know at least one linker relies upon for code size.

My understanding is that -fdata-sections is just an option that affects the section selection for a global. My intent was that this change would only affect globals that have been assigned to a specific section via an attribute or the section property... to put it another way there appears to ab no effect on the IR from adding -fdata-sections to clang so I don't understand how this change could effect this... no?

Hello James! Thanks for informing us. If this does what you say, it essentially disables global merging with -fdata-sections, then indeed we would get a little bit unhappy about that.
I could run some numbers with this patch, but I can guess what the outcome will be..... I haven't looked at this patch or the PR, but does this need to be default behaviour, or can this e.g. be done under an option?

I can see that this could cause performance problems if you were relying on the merging. I feel strongly that the behaviour in this patch (which matches GCC) should be the default, however I don't really have an objection to adding an option to re-enable merging. Please have a look at https://reviews.llvm.org/D68147 where I have attempted to do that.

I am getting up to speed with this... and created this reproducer:

static int A[4] __attribute__ ((section ("HELLO"))) = {1,0,0,1} ;
static long long B[9]  __attribute__ ((section ("HELLO"))) = {0,0,0,0,0,0,0,0,1};
int foo (int i) {
  return A[i] + B[i];
}

The corresponding ELF object contains this section:

Name        : HELLO
Type        : SHT_PROGBITS (0x00000001)
Flags       : SHF_ALLOC + SHF_MERGE (0x00000012)
Addr        : 0x00000000
File Offset : 96 (0x60)
Size        : 88 bytes (0x58)
Link        : SHN_UNDEF
Info        : 0
Alignment   : 8
Entry Size  : 16

You're unhappy about the Entry Size : 16, as this sections contains entries of words and double-words. The ELF spec says this about this entry:

The size of each element is specified in the section header's sh_entsize field.

So yes, I guess that means the entry size needs to be uniform.

Now my question about this patch: why are we trying to patch things up when we can avoid putting incompatible symbols in the same section in the first place? In this case, the merging is achieved with a section attribute, but from the PR I understood the same can be achieved with a pragma, so from that point of view we perhaps got what we deserved? Would be nice though to warn about this, and/or the pass that does the merging should not do this?

Hi Sjoerd,

Nice reproducer.

In this case, the merging is achieved with a section attribute, but from the PR I understood the same can be achieved with a pragma,

The frontend attribute/pragmas that I am aware of are:

#pragma clang section ... - https://clang.llvm.org/docs/LanguageExtensions.html#specifying-section-names-for-global-objects-pragma-clang-section
and
_attribute_ ((section ("section-name"))) - https://clang.llvm.org/docs/AttributeReference.html#section-declspec-allocate

Now my question about this patch: why are we trying to patch things up when we can avoid putting incompatible symbols in the same section in the first place? In this case, the merging is achieved with a section attribute, but from the PR I understood the same can be achieved with a pragma, so from that point of view we perhaps got what we deserved? Would be nice though to warn about this, and/or the pass that does the merging should not do this?

This and https://reviews.llvm.org/D68147 are attempts at minimal patches to address just the mergeable globals issue. The larger issue is how should incompatible symbols and attribute ((section ("<section name>")) interact? I have outlined some possible fixes on the bugzilla; but, the problem is that I am not sure of the intent of the attribute. It seems reasonable that in the case of your example the user expected a single HELLO section in the output containing both symbols. With this interpretation our choices are to try to create such a section or fail if we can't. However, perhaps the user would be happy with two sections called HELLO in the output.. in which case it we could create multiple output sections with the name HELLO each containing a set of compatible symbols. So basically to proceed we need to decide on the correct semantics for this attribute. We also need to evaluate real world use cases for this attributes.. e.g. I suspect this is often combined with inline asm... in these cases users might be relying on implementation specific behaviour and fixing this bug might break their code. Also the design of MC makes some fixes difficult to implement, e.g. it is single pass so I can't fix up the section attributes when I encounter an incompatible global. I think a full fix for this issue is difficult which is why I have only put up a fix for the mergable part of the bugzilla.

As to why I am attempting to patch things up rather than do something early... in this case there is no optimizer pass that does this merging optimization. It is an MC optimization. AFAIK the fix needs to be in MC. I think that this patch or https://reviews.llvm.org/D68147 are reasonable attempts at a fix since they get us closer to the GCC behaviour which seems to be:

Create a single section if possible: https://godbolt.org/z/yKVNUd
Error if it is not possible: https://godbolt.org/z/rn7o7a

p.s. GCC's error messages are better than what are produced by https://reviews.llvm.org/D68147; but, I don't seem to have access to enough information in MC to produce a better message.

Hi Ben, this is not really my area of expertise, but it all starts to make some sense to me. I was expecting this transformation to happen earlier, but this is where the magic happens, and this probably where it belongs.
Just to make sure I haven't missed anything, I would like to take this patch and run some numbers, for which I need a little bit of time. If in the mean time someone with some more experience in this area here has a look too, that would be great....

bd1976llvm added a subscriber: mcgrathr.EditedOct 1 2019, 10:15 AM

Hi Ben, this is not really my area of expertise, but it all starts to make some sense to me. I was expecting this transformation to happen earlier, but this is where the magic happens, and this probably where it belongs.
Just to make sure I haven't missed anything, I would like to take this patch and run some numbers, for which I need a little bit of time. If in the mean time someone with some more experience in this area here has a look too, that would be great....

Thanks. I am not sure who can review this. I added people form the "#pragma clang section" reviews. I will add @rjmccall as this is a frontend interacting with linking issue.

bd1976llvm added a subscriber: cfe-commits.