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

Fri Feb 28 09:55:54 PST 2020

MaskRay added a comment.

@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.
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.

> 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.

> 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.

> 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.

> 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72892