[PATCH] D72892: [MC][ARM] Resolve some pcrel fixups at assembly time

Sun Mar 1 08:10:42 PST 2020

MaskRay added a comment.

In D72892#1899873 <https://reviews.llvm.org/D72892#1899873>, @strejda wrote:

> In D72892#1898432 <https://reviews.llvm.org/D72892#1898432>, @MaskRay wrote:
>
> > @strejda Apologies for any problems caused, but I am afraid I cannot agree with all the points.
> >
> > In D72892#1898224 <https://reviews.llvm.org/D72892#1898224>, @strejda wrote:
> >
> > > hmm, I discussed with MC developer on #bsdmips :). Hope he can make MC more conform to widely accepted standards, without issuing more and more regressions, and to understand that:
> > >
> > > 1. The code in locore_v6.S is absolutely valid. It is used in strictly statically linked environment to fixed address, so every reference to interposed symbol handling is absurd.
> >
> >
> > `adr r0, foo` When `foo` is STB_LOCAL, this is true. When `foo` is STB_GLOBAL, I have to say it is fuzzy.
>
>
> Nope, in almost all cases correct behavior is evident, the only remaining question is which variants (combinations) are supported and which are not.. You can't change reality by denying arguments that don't match with your theory.  So, assume that `foo` is STB_GLOBAL for rest of this discussion.
>  Then, please, tell me what exactly is fuzzy if the above code is used in 
>  a) static environment or 
>  b) dynamic environment but given symbol is not interposed.  
>  I have asked for this several times, always without response. And, please, be very exact and precise – this is key point (at least for me).
>
> My answer is very clear and clear - the behavior in all cases is very well defined. You can identify incorrect cases and report errors, but you cannot broke legal use. It's always better if the compiler silently accepts the wrong code rather than rejecting a valid one. So again and again – You broke perfectly valid code.

I have argued many times it is not clear the "label" can be STB_GLOBAL for such short range fixups, but receive no response. If not, they are not "very well defined". It is not a problem that a conforming assembler does not support such STB_GLOBAL labels.

>> If it is convenient for you, please estimate how much code uses STB_GLOBAL. It will be more persuasive than simply calling it "a regression". If a code problem is widespread, then toolchain may still have to work around them.
> 
> Huh, isn't that exactly what you should have done before you broke given status quo? But if the android, freebsd and linux kernel is not enough (you broke all them, right?),  be sure that using “adr” on the global symbols defined in the same section is a common way to describe the various data structures in assembler and then use them in C.

So far Android and ChromeOS are good. "freebsd and linux kernel is not enough" =>
I have checked this does not affect compiler generated code. There is one assembly instance in Linux. I am helping ClangBuiltLinux, so I'll help investigate and fix it anyway. They are cooperative if there are indeed issues from the kernel side. (But of course, there can be stubborn opinions in the Linux kernel community, and sometimes the toolchain has to cope.)

>>> 2. MC cannot, in any case, refuse to compile valid code only because same code can be invalid in other environment.
>> 
>> There is a widely agreed rule: a direct access (non-GOT non-PLT) relocation referencing a STV_DEFAULT STB_GLOBAL symbol is not allowed. Deviating from it is considered incorrect.
>>  In this context, GNU as does not produce these short range relocations. Instead (inconsistent with other ports of GNU as), such fixups are resolved at assembly time.
>>  Such mistakes can be easily made: binutils code does not have a good abstraction layer for relocation types on different architectures. It is easy to omit something for some relocation types.
>>  For example, the PowerPC arch maintainer has admitted R_PPC_ADDR16_LO is a problem, though they will probably refuse fixing the problem.
>>  I don't have access to an ARM assembler and don't know how it behaves.
> 
> All ARM assemblers was consistent before your change.
>  Give me at least some sort of evidence of this “widely agreed rule“ and mainly about it applicability to assembler code.
> 
>   So you want say that every line in following example is wrong in every environment? In that case be sure that all projects (ports) are broken.
>    
> 
>           .text
>           .globl foo
>   foo:
>           ldr     r0, =foo
>           adr     r1, foo
>           movw    r2,#:lower16:foo
>           movt    r2,#:upper16:foo
>           b       foo
>           .word   foo        
> 
> 
> As you can see I wrote "valid code". Of course, you may name "valid code" as "undefined behavior", but this doesn't mean that all others will be on same wave. For me and all FreeBSD developers, the locore-v6.S code is fully valid and you haven't been able to tell why exactly that code is invalid.

Please measure, instead of asserting "all projects (ports) are broken".

All ARM assemblers was consistent => it does not necessarily mean they are all correct.
https://reviews.llvm.org/D75416 => Both clang and GCC violate a clear ELF specification by not placing .toc in a section group if required => GNU ld happily accepts it.
nullptr cannot be implicitly converted to Wrap<bool> => at some point both clang and GCC were wrong => GCC fixed it => clang fixed it a few weeks ago. I can argue before GCC fixed it => all (probably not all, but close) C++ compilers were consistent, but that can't be the reason to make a change.

>>> 3. Situation when new version of given program fails for valid sources, but previous version worked as expected, is called regression.
>> 
>> It can be an undefined behavior in metaqwe's words. clang can be loose and incorrectly allow some code constructs while a future release may disallow them.
> 
> As you can see I wrote "valid sources". Of course, you may name "valid sources" as "undefined behavior", but this doesn't mean that all others will be on same wave. For me and all FreeBSD developers, the locore-v6.S code is fully valid and you haven't been able to tell why exactly that code is invalid.
> 
>> 
>> 
>>> 4. Under standard open source policy, the regression is always release blocker and it cannot be resolved as won’t fix.
>> 
>> Every regression should be carefully treated, but for this case it is hard to say this issue is a regression.
> 
> Again nope, all others says you that this is regression. The situation is very clear in this point.

Your starting point is not clear => it is not clear the issue is a regression in the first place.

>>> 5. Marking regression as won’t fix is synonym for “f**k with users”.
>>> 
>>>   Just look, I fully understand that symbol interposition is “problematic” in these cases. However you cannot, in any case, expect that given assembly unit will be used in environment where symbol interposition is allowed. Or worse, you can't expect that given symbol will be interposed. All you can do is to issue a warning if there is a high probability that the problem may occur. Or issue an error if the problem has already occurred. Nothing more.
>>> 
>>>   I apologize for this rant talk, but this issue makes me really angry. Imho, this is exact reason why MC have very bad reputation in arm community, why is very problematic to pass patch to upstream with "this code is not compiled by MC but is valid for all others assemblers". And like always - we (FreeBSD) cannot compile more than 2/3 of ports with assembler files  just because MC doesn't support  pre-UAL syntax and upstream doesn't want to switch to UAL.  This is very frustrating situation.
>> 
>> The other ARM assembly issues are unrelated to this particular short range relocation issue. I have the viewpoint that we should be careful accepting new features. It is not uncommon to bring legacy stuff into a project which does not play well with other things. Accepting legacy stuff into a compiler/assembler/linker may be easy to paper over some code issues, but it may not be healthy for the project in the long term.
> 
> Hahaha, this is joke, right? Or you really thinks that rejection of 2/3 of existing code is sign of healthy project? If so, I am afraid you are the only one who has this opinion. And you don't know how much time this decision costs to others (me including)
> 
>> For this case, Peter kindly offers help implementing these relocation types. They will make the overall assembler+linker model sane.
> 
> I agree with thas, so Peter@ many many many thanks !!!
> 
>> Though, I am nervous about the complexity. I don't know enough of ARM and I don't know how to estimate the code which actually uses such short range relocations referencing STB_GLOBAL symbols.
> 
> But this complexity is direct result of your incorrect assumptions and patch, sorry.
> Michal

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72892