[llvm] r233119 - AArch64: use a different means to determine whether to byte swap relocations.

[Rafael Espíndola]

I think you analysis in correct.

Unfortunately that means there is a bug in the code (before and
after): https://llvm.org/bugs/show_bug.cgi?id=23020

On 24 March 2015 at 19:33, Peter Collingbourne <peter at pcc.me.uk> wrote:
> I still haven't figured out how to test this. For example:
>
> .section .data
> .word x + .Lbar - (.Lfoo + x)
> .word .Lbar - .Lfoo
> .word (.Lbar - .Lfoo) + 1
> .word 1 + (.Lbar - .Lfoo)
> .word (.Lbar - .Lfoo) - 1
> .word .Lbar2 - .Lfoo2
>
> .section .eh_frame
> .Lfoo:
> .byte 1
> .Lbar:
> .byte 1
>
> .section .not_eh_frame
> .Lfoo2:
> .byte 1
> .Lbar2:
> .byte 1
>
> (test with "llvm-mc -triple=aarch64_be-linux-gnu -filetype=obj -o - eh-frame.s")
>
> In all of these cases, the new code produces the same result. It seems
> that the effects are only visible if Value is non-zero, which it will
> only be if both symbols are in the same section. But if that is the case,
> EvaluateAsRelocatable will return the same result.
>
> Peter
>
> On Tue, Mar 24, 2015 at 06:10:27PM -0400, Rafael Espíndola wrote:
>> Oh, and the code should be checking SymB too.
>>
>> A logic similar to getBaseSymbol is probably what is needed.
>>
>> On 24 March 2015 at 18:03, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
>> > This is testable. The new code should be able to handle more situations.
>> >
>> > On 24 March 2015 at 17:47, Peter Collingbourne <peter at pcc.me.uk> wrote:
>> >> Author: pcc
>> >> Date: Tue Mar 24 16:47:03 2015
>> >> New Revision: 233119
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=233119&view=rev
>> >> Log:
>> >> AArch64: use a different means to determine whether to byte swap relocations.
>> >>
>> >> This code depended on a bug in the FindAssociatedSection function that would
>> >> cause it to return the wrong result for certain absolute expressions. Instead,
>> >> use EvaluateAsRelocatable.
>> >>
>> >> Modified:
>> >>     llvm/trunk/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp
>> >>
>> >> Modified: llvm/trunk/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp
>> >> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp?rev=233119&r1=233118&r2=233119&view=diff
>> >> ==============================================================================
>> >> --- llvm/trunk/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp (original)
>> >> +++ llvm/trunk/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp Tue Mar 24 16:47:03 2015
>> >> @@ -18,6 +18,7 @@
>> >>  #include "llvm/MC/MCObjectWriter.h"
>> >>  #include "llvm/MC/MCSectionELF.h"
>> >>  #include "llvm/MC/MCSectionMachO.h"
>> >> +#include "llvm/MC/MCValue.h"
>> >>  #include "llvm/Support/ErrorHandling.h"
>> >>  #include "llvm/Support/MachO.h"
>> >>  using namespace llvm;
>> >> @@ -493,14 +494,28 @@ void ELFAArch64AsmBackend::processFixupV
>> >>      IsResolved = false;
>> >>  }
>> >>
>> >> +// Returns whether this fixup is based on an address in the .eh_frame section,
>> >> +// and therefore should be byte swapped.
>> >> +// FIXME: Should be replaced with something more principled.
>> >> +static bool isByteSwappedFixup(const MCExpr *E) {
>> >> +  MCValue Val;
>> >> +  if (!E->EvaluateAsRelocatable(Val, nullptr, nullptr))
>> >> +    return false;
>> >> +
>> >> +  if (!Val.getSymA() || Val.getSymA()->getSymbol().isUndefined())
>> >> +    return false;
>> >> +
>> >> +  const MCSectionELF *SecELF =
>> >> +      dyn_cast<MCSectionELF>(&Val.getSymA()->getSymbol().getSection());
>> >> +  return SecELF->getSectionName() == ".eh_frame";
>> >> +}
>> >> +
>> >>  void ELFAArch64AsmBackend::applyFixup(const MCFixup &Fixup, char *Data,
>> >>                                        unsigned DataSize, uint64_t Value,
>> >>                                        bool IsPCRel) const {
>> >>    // store fixups in .eh_frame section in big endian order
>> >>    if (!IsLittleEndian && Fixup.getKind() == FK_Data_4) {
>> >> -    const MCSection *Sec = Fixup.getValue()->FindAssociatedSection();
>> >> -    const MCSectionELF *SecELF = dyn_cast_or_null<const MCSectionELF>(Sec);
>> >> -    if (SecELF && SecELF->getSectionName() == ".eh_frame")
>> >> +    if (isByteSwappedFixup(Fixup.getValue()))
>> >>        Value = ByteSwap_32(unsigned(Value));
>> >>    }
>> >>    AArch64AsmBackend::applyFixup (Fixup, Data, DataSize, Value, IsPCRel);
>> >>
>> >>
>> >> _______________________________________________
>> >> llvm-commits mailing list
>> >> llvm-commits at cs.uiuc.edu
>> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
> --
> Peter