[PATCH] D35944: [ELF] Disable relocation validation when targeting weak undefined symbols

[James Henderson via Phabricator via llvm-commits]

jhenderson abandoned this revision.
jhenderson added a comment.

Having considered this, and looked at the latest behaviour of ld.bfd and gold (they both emit an error in this case), I think this change is not necessary after all.



================
Comment at: ELF/InputSection.cpp:731-733
+      bool UndefinedWeak = (Rel.Sym->isUndefined() || Rel.Sym->isLazy()) &&
+                           !Rel.Sym->isLocal() && Rel.Sym->symbol()->isWeak();
+      Target->relocateOne(BufLoc, Type, TargetVA, !UndefinedWeak);
----------------
davidb wrote:
> davidb wrote:
> > jhenderson wrote:
> > > jhenderson wrote:
> > > > ruiu wrote:
> > > > > I wonder if you really have to change this many places to handle weak undefined symbols. If you don't call `relocateOne` if `UndefinedWeak` is true, doesn't it work?
> > > > I'm happy to do that. The only reason I did it this way was to match ld.bfd and gold's behaviour in this situation, but I don't think it really matters (the call should never be executed anyway).
> > > I've looked at making a version of this change that does what you suggested. However, this alternative behaviour breaks the guarantee provided by the ELF specification, that undefined weak symbols are treated as having a value of zero (ignoring the ARM special case). I'm concerned that there might be some issue with a corner case that relies on this behaviour.
> > > 
> > > The end result of this alternative change would be that either zero or the addend would be left in place in the instruction (depending on whether RELA or REL relocations are used), as opposed to zero or a value relative to zero (depending on whether we are dealing with an absolute or relative relocation).
> > The change as it currently stands doesn't necessarily guarantee the result of the relocation patch is zero? 
> > 
> > Can't you just enforce TargetVA to 0 when invoking relocateOne?
> With the ARM special case, are you referring to PC-biasing?
> The change as it currently stands doesn't necessarily guarantee the result of the relocation patch is zero?
This isn't what is required by the standard, and isn't what is being attempted here. The idea is that the relocation is performed as if the virtual address of the target is zero, so a non-zero value will usually end up being patched in for relative relocations. An exception is that on ARM/AARCH64, PC-relative references are patched such that the branch etc ends up jumping to the next instruction (see getARMUndefinedRelativeWeakVA in LLD's code base).

Unfortunately treating a symbol as having a value of zero when the rest of the program is at a very high address can lead to out-of-range errors, hence the purpose of this change.


https://reviews.llvm.org/D35944