[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

Tue Oct 1 03:46:15 PDT 2019

bd1976llvm added a comment.

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.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68101/new/

https://reviews.llvm.org/D68101