[MC] Error for .globl/.local which change the symbol binding and warn for .weak

[MaskRay]

Summary

GNU as let .weak override .globl since binutils-gdb
5ca547dc2399a0a5d9f20626d4bf5547c3ccfddd (1996) while MC lets the last
directive win (PR38921).

This caused an issue to Linux's powerpc port which has been fixed by
http://git.kernel.org/linus/968339fad422a58312f67718691b717dac45c399

Binding overriding is error-prone. This patch disallows a changed binding.
(https://sourceware.org/pipermail/binutils/2020-March/000299.html )

Our behavior regarding .globl x; .weak x matches GNU as. Such usage is
still suspicious but we issue a warning for now. We may upgrade it to an
error in the future.

[psmith]

Personally I'd err on the side of consistency with binutils. If they are happy to go to an error then we can. If they aren't then I recommend that we warn rather than error as we risk breaking builds for working software and it isn't always easy to get source code fixed. Not got a strong opinion though so happy to go with the consensus.

[nickdesaulniers]

I wonder how many Linux kernel targets will break when using LLVM_IAS=1 with this? We should be able to fix up any instances we find in kernel sources though. Thanks for the patch.

[MaskRay]

In D90108#2354374, @nickdesaulniers wrote:

I wonder how many Linux kernel targets will break when using LLVM_IAS=1 with this? We should be able to fix up any instances we find in kernel sources though. Thanks for the patch.

ClangBuiltLinux/tc-build

cd kernel; ./build.sh -t X86 -s path/to/my/linux -p path/to/my/llvm/build/bin succeeds. There are probably not many.

In D90108#2354373, @psmith wrote:

Personally I'd err on the side of consistency with binutils. If they are happy to go to an error then we can. If they aren't then I recommend that we warn rather than error as we risk breaking builds for working software and it isn't always easy to get source code fixed. Not got a strong opinion though so happy to go with the consensus.

If we issue a warning instead, users will only notice the semantic difference when they use -Wa,--fatal-warnings. We can classify the potential risks on whether .local is used:

• .local is used: .local is a very rare directive.
• .local is not used: the issue is about .weak and .globl . This represents a semantic difference between MC and GNU ld. There may be a few more but I doubt there can be more than a few. Can we try error first and switch to a warning if it turns out to be a problem?

[psmith]

If we issue a warning instead, users will only notice the semantic difference when they use -Wa,--fatal-warnings. We can classify the potential risks on whether .local is used:

• .local is used: .local is a very rare directive.
• .local is not used: the issue is about .weak and .globl . This represents a semantic difference between MC and GNU ld. There may be a few more but I doubt there can be more than a few. Can we try error first and switch to a warning if it turns out to be a problem?

I don't object to go for an error; I do tend to be a bit nervous about making something an error that has been accepted for many years as contrary to my expectations that something is rare, there is almost always someone that has a special case and is greatly inconvenienced by it. If we are lucky very few people will be affected and they can change their code, if we are not then we may have to revisit.

[MaskRay]

In D90108#2355685, @psmith wrote:

If we issue a warning instead, users will only notice the semantic difference when they use -Wa,--fatal-warnings. We can classify the potential risks on whether .local is used:

• .local is used: .local is a very rare directive.
• .local is not used: the issue is about .weak and .globl . This represents a semantic difference between MC and GNU ld. There may be a few more but I doubt there can be more than a few. Can we try error first and switch to a warning if it turns out to be a problem?

I don't object to go for an error; I do tend to be a bit nervous about making something an error that has been accepted for many years as contrary to my expectations that something is rare, there is almost always someone that has a special case and is greatly inconvenienced by it. If we are lucky very few people will be affected and they can change their code, if we are not then we may have to revisit.

Thanks for the insight! This is something I have thought. .local is rare so this is about the semantic differences between .weak x;.globl x and .globl x;.weak x. Since we actually do different thing, and if the user's intention was to get a STB_WEAK, the current behavior may make them surprised. So an error is probably fine since we conjecture the number of instances is very few. We can surely loose it if it turns out to be a wide spreading problem.

[nickdesaulniers]

In D90108#2357336, @MaskRay wrote:

We can surely loose it if it turns out to be a wide spreading problem.

The hard part is that regressions tend to get reported AFTER the release when it's too late to fix them. Maybe a warning for one release, then upgrade to hard error next release gives users more time to find and fix errors, and doesn't result in an unusable release version for anyone?

[MaskRay]

In D90108#2357342, @nickdesaulniers wrote:

In D90108#2357336, @MaskRay wrote:

We can surely loose it if it turns out to be a wide spreading problem.

The hard part is that regressions tend to get reported AFTER the release when it's too late to fix them. Maybe a warning for one release, then upgrade to hard error next release gives users more time to find and fix errors, and doesn't result in an unusable release version for anyone?

We other kinds of diagnostics I might agree, for this particular one I think using a warning is a bit unnecessarily conservative.

[jhenderson]

In D90108#2357342, @nickdesaulniers wrote:

In D90108#2357336, @MaskRay wrote:

We can surely loose it if it turns out to be a wide spreading problem.

The hard part is that regressions tend to get reported AFTER the release when it's too late to fix them. Maybe a warning for one release, then upgrade to hard error next release gives users more time to find and fix errors, and doesn't result in an unusable release version for anyone?

I don't have any specific evidence to suggest that we'd run into a problem doing this, but my personal thinking is that adding an option to allow disabling this error might be a good idea. I wouldn't be surprised to find code bases where fixing the code is non-trivial. Ideally, the option would not be made widely known unless actually asked for and/or heavily labelled with things saying something like "this is a temporary option that will be deleted in a future change unless a compelling reason is found not to".

[MaskRay]

In D90108#2358482, @jhenderson wrote:

In D90108#2357342, @nickdesaulniers wrote:

In D90108#2357336, @MaskRay wrote:

We can surely loose it if it turns out to be a wide spreading problem.

The hard part is that regressions tend to get reported AFTER the release when it's too late to fix them. Maybe a warning for one release, then upgrade to hard error next release gives users more time to find and fix errors, and doesn't result in an unusable release version for anyone?

I don't have any specific evidence to suggest that we'd run into a problem doing this, but my personal thinking is that adding an option to allow disabling this error might be a good idea. I wouldn't be surprised to find code bases where fixing the code is non-trivial. Ideally, the option would not be made widely known unless actually asked for and/or heavily labelled with things saying something like "this is a temporary option that will be deleted in a future change unless a compelling reason is found not to".

If there are concerns, I can make .globl foo; .weak foo a warning instead of error. The others are still errors.

[psmith]

In D90108#2359539, @MaskRay wrote:

In D90108#2358482, @jhenderson wrote:

In D90108#2357342, @nickdesaulniers wrote:

In D90108#2357336, @MaskRay wrote:

We can surely loose it if it turns out to be a wide spreading problem.

The hard part is that regressions tend to get reported AFTER the release when it's too late to fix them. Maybe a warning for one release, then upgrade to hard error next release gives users more time to find and fix errors, and doesn't result in an unusable release version for anyone?

I don't have any specific evidence to suggest that we'd run into a problem doing this, but my personal thinking is that adding an option to allow disabling this error might be a good idea. I wouldn't be surprised to find code bases where fixing the code is non-trivial. Ideally, the option would not be made widely known unless actually asked for and/or heavily labelled with things saying something like "this is a temporary option that will be deleted in a future change unless a compelling reason is found not to".

If there are concerns, I can make .globl foo; .weak foo a warning instead of error. The others are still errors.

I'm happy for that to be a warning, or an option to disable the error. As mentioned in the first comment, it is only a weakly held opinion.

[MaskRay]

Use a warning if the binding is changed to STB_WEAK

[jhenderson]

Looks good to me, with nits.

llvm/lib/MC/MCELFStreamer.cpp

230

240

(for consistency with the comment above)

[MaskRay]

Comments

llvm/lib/MC/MCELFStreamer.cpp

bool MCELFStreamer::emitSymbolAttribute(MCSymbol *S, MCSymbolAttr Attribute) {
 
   case MCSA_ELF_TypeGnuUniqueObject:
     Symbol->setType(CombineSymbolTypes(Symbol->getType(), ELF::STT_OBJECT));
     Symbol->setBinding(ELF::STB_GNU_UNIQUE);
     Symbol->setExternal(true);
     break;
 
   case MCSA_Global:
+    // For `.weak x; .global x`, GNU as sets the binding to STB_WEAK while we
+    // traditionally set the binding to STB_GLOBAL. This is error-prone, so we
+    // error on such cases. Note, we also disallow changed binding from .local.
+    if (Symbol->isBindingSet() && Symbol->getBinding() != ELF::STB_GLOBAL)
+      getContext().reportError(SMLoc(), Symbol->getName() +
+                                            " changed binding to STB_GLOBAL");
     Symbol->setBinding(ELF::STB_GLOBAL);
     Symbol->setExternal(true);
     break;
 
   case MCSA_WeakReference:
   case MCSA_Weak:
+    // For `.global x; .weak x`, both MC and GNU as set the binding to STB_WEAK.
+    // We emit a warning for now but may switch to an error in the future.
+    if (Symbol->isBindingSet() && Symbol->getBinding() != ELF::STB_WEAK)
+      getContext().reportWarning(SMLoc(), Symbol->getName() +
+                                              " changed binding to STB_WEAK");
     Symbol->setBinding(ELF::STB_WEAK);
     Symbol->setExternal(true);
     break;
 
   case MCSA_Local:
+    if (Symbol->isBindingSet() && Symbol->getBinding() != ELF::STB_LOCAL)
+      getContext().reportError(SMLoc(), Symbol->getName() +
+                                            " changed binding to STB_LOCAL");
     Symbol->setBinding(ELF::STB_LOCAL);
     Symbol->setExternal(false);
     break;
 
   case MCSA_ELF_TypeFunction:
     Symbol->setType(CombineSymbolTypes(Symbol->getType(), ELF::STT_FUNC));
     break;
 

llvm/test/MC/ELF/symbol-binding-changed.s

+# RUN: not llvm-mc -filetype=obj -triple=x86_64 %s -o /dev/null 2>&1 | FileCheck %s --implicit-check-not=error:
+
+# CHECK: error: local changed binding to STB_GLOBAL
+local:
+.local local
+.globl local
+
+## `.globl x; .weak x` matches the GNU as behavior. We issue a warning for now.
+# CHECK: warning: global changed binding to STB_WEAK
+global:
+.global global
+.weak global
+
+# CHECK: error: weak changed binding to STB_LOCAL
+weak:
+.weak weak
+.local weak
+
+# CHECK-NOT: error:
+multi_local:
+.local multi_local
+.local multi_local
+multi_global:
+.global multi_global
+.global multi_global
+multi_weak:
+.weak multi_weak
+.weak multi_weak