[PATCH] D90108: [MC] Error for .weak/.globl/.local which change the symbol binding

Wed Oct 28 13:35:05 PDT 2020

psmith added a comment.

In D90108#2359539 <https://reviews.llvm.org/D90108#2359539>, @MaskRay wrote:

> In D90108#2358482 <https://reviews.llvm.org/D90108#2358482>, @jhenderson wrote:
>
>> In D90108#2357342 <https://reviews.llvm.org/D90108#2357342>, @nickdesaulniers wrote:
>>
>>> In D90108#2357336 <https://reviews.llvm.org/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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90108