[llvm-commits] [llvm] r123294 - in /llvm/trunk: lib/MC/ELFObjectWriter.cpp lib/Target/ARM/ARMAsmBackend.cpp lib/Target/ARM/ARMAsmPrinter.cpp lib/Target/ARM/ARMFixupKinds.h lib/Target/ARM/ARMMCCodeEmitter.cpp test/MC/ARM/elf-movt.s
Jason Kim
jasonwkim at google.com
Wed Jan 12 14:34:48 PST 2011
On Wed, Jan 12, 2011 at 1:56 PM, Jim Grosbach <grosbach at apple.com> wrote:
> Observations below.
>
> On Jan 11, 2011, at 4:19 PM, Jason W Kim wrote:
>
>> Author: jasonwkim
>> Date: Tue Jan 11 18:19:25 2011
>> New Revision: 123294
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=123294&view=rev
>> Log:
>> 1. Support ELF pcrel relocations for movw/movt:
>> R_ARM_MOVT_PREL and R_ARM_MOVW_PREL_NC.
>> 2. Fix minor bug in ARMAsmPrinter - treat bitfield flag as a bitfield, not an enum.
>> 3. Add support for 3 new elf section types (no-ops)
>>
>>
>>
>> Modified:
>> llvm/trunk/lib/MC/ELFObjectWriter.cpp
>> llvm/trunk/lib/Target/ARM/ARMAsmBackend.cpp
>> llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp
>> llvm/trunk/lib/Target/ARM/ARMFixupKinds.h
>> llvm/trunk/lib/Target/ARM/ARMMCCodeEmitter.cpp
>> llvm/trunk/test/MC/ARM/elf-movt.s
>>
>> Modified: llvm/trunk/lib/MC/ELFObjectWriter.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/ELFObjectWriter.cpp?rev=123294&r1=123293&r2=123294&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/MC/ELFObjectWriter.cpp (original)
>> +++ llvm/trunk/lib/MC/ELFObjectWriter.cpp Tue Jan 11 18:19:25 2011
>> @@ -1268,6 +1268,9 @@
>> case ELF::SHT_NOTE:
>> case ELF::SHT_NULL:
>> case ELF::SHT_ARM_ATTRIBUTES:
>> + case ELF::SHT_INIT_ARRAY:
>> + case ELF::SHT_FINI_ARRAY:
>> + case ELF::SHT_PREINIT_ARRAY:
>> // Nothing to do.
>> break;
>>
>> @@ -1490,6 +1493,13 @@
>> default:
>> Type = ELF::R_ARM_CALL; break;
>> } break;
>> + case ARM::fixup_arm_movt_hi16:
>> + case ARM::fixup_arm_movt_hi16_pcrel:
>> + Type = ELF::R_ARM_MOVT_PREL; break;
>> + case ARM::fixup_arm_movw_lo16:
>> + case ARM::fixup_arm_movw_lo16_pcrel:
>> + Type = ELF::R_ARM_MOVW_PREL_NC; break;
>> +
>
> Should the pc-rel and non-pc-rel fixups really both map to the same relocation? That doesn't seem right. If they're the same, they shouldn't need distinct fixups. If they're different, they should map differently.
Hmm, it does look funny - I have to track down the local test cases
that caused me to code it thus - I'll look into it now. I fear it may
be an artifact of emulating GNU as :-(
>
> The "break" statement should be on the following line, not the same line as the assignment. As a style thing, keeping the break on the same line is fine when the whole thing (including the case label) is on the same line for simple stuff. Once
> there's already multiple lines, please separate them. The code above this gets very confusing due to this with nested switch statements and multiple breaks and such.
Yes. Will do!
>
>
>> }
>> } else {
>> switch ((unsigned)Fixup.getKind()) {
>>
>> Modified: llvm/trunk/lib/Target/ARM/ARMAsmBackend.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMAsmBackend.cpp?rev=123294&r1=123293&r2=123294&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Target/ARM/ARMAsmBackend.cpp (original)
>> +++ llvm/trunk/lib/Target/ARM/ARMAsmBackend.cpp Tue Jan 11 18:19:25 2011
>> @@ -78,6 +78,8 @@
>> { "fixup_arm_thumb_bcc", 1, 8, MCFixupKindInfo::FKF_IsPCRel },
>> { "fixup_arm_movt_hi16", 0, 16, 0 },
>> { "fixup_arm_movw_lo16", 0, 16, 0 },
>> +{ "fixup_arm_movt_hi16_pcrel", 0, 16, MCFixupKindInfo::FKF_IsPCRel },
>> +{ "fixup_arm_movw_lo16_pcrel", 0, 16, MCFixupKindInfo::FKF_IsPCRel },
>> };
>>
>> if (Kind < FirstTargetFixupKind)
>> @@ -156,7 +158,9 @@
>> case FK_Data_4:
>> return Value;
>> case ARM::fixup_arm_movt_hi16:
>> - case ARM::fixup_arm_movw_lo16: {
>> + case ARM::fixup_arm_movw_lo16:
>> + case ARM::fixup_arm_movt_hi16_pcrel:
>> + case ARM::fixup_arm_movw_lo16_pcrel: {
>> unsigned Hi4 = (Value & 0xF000) >> 12;
>> unsigned Lo12 = Value & 0x0FFF;
>> // inst{19-16} = Hi4;
>>
>> Modified: llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp?rev=123294&r1=123293&r2=123294&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp (original)
>> +++ llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp Tue Jan 11 18:19:25 2011
>> @@ -189,10 +189,10 @@
>> int64_t Imm = MO.getImm();
>> O << '#';
>> if ((Modifier && strcmp(Modifier, "lo16") == 0) ||
>> - (TF == ARMII::MO_LO16))
>> + (TF & ARMII::MO_LO16))
>> O << ":lower16:";
>> else if ((Modifier && strcmp(Modifier, "hi16") == 0) ||
>> - (TF == ARMII::MO_HI16))
>> + (TF & ARMII::MO_HI16))
>
> As previously mentioned, you can just nuke the "Modifier" string compare. It's obsolete.
>
> The value-kinds look like masks, but they're not consistently used that way, unfortunately. If you want to change them to work additively instead of a one-variant-kind-per-symbolref, that's great and I completely agree it's worth doing, but it really should be a separate patch. Until then, this isn't correct. Please revert this bit.
>
>> O << ":upper16:";
>> O << Imm;
>> break;
>>
>> Modified: llvm/trunk/lib/Target/ARM/ARMFixupKinds.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMFixupKinds.h?rev=123294&r1=123293&r2=123294&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Target/ARM/ARMFixupKinds.h (original)
>> +++ llvm/trunk/lib/Target/ARM/ARMFixupKinds.h Tue Jan 11 18:19:25 2011
>> @@ -74,6 +74,11 @@
>> fixup_arm_movt_hi16, // :upper16:
>> fixup_arm_movw_lo16, // :lower16:
>>
>> + // It is possible to create an "immediate" that happens to be pcrel.
>
> I don't follow. Can you add an example?
See below.
>
>> + // Needed to support ELF::R_ARM_MOVT_PREL and ELF::R_ARM_MOVW_PREL_NC
>> + fixup_arm_movt_hi16_pcrel, // :upper16:
>> + fixup_arm_movw_lo16_pcrel, // :lower16:
>> +
>> // Marker
>> LastTargetFixupKind,
>> NumTargetFixupKinds = LastTargetFixupKind - FirstTargetFixupKind
>>
>> Modified: llvm/trunk/lib/Target/ARM/ARMMCCodeEmitter.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMMCCodeEmitter.cpp?rev=123294&r1=123293&r2=123294&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Target/ARM/ARMMCCodeEmitter.cpp (original)
>> +++ llvm/trunk/lib/Target/ARM/ARMMCCodeEmitter.cpp Tue Jan 11 18:19:25 2011
>> @@ -626,6 +626,32 @@
>> return Binary;
>> }
>>
>> +// FIXME: This routine needs to handle more MCExpr types
>> +static const MCSymbolRefExpr *FindLHSymExpr(const MCExpr *E) {
>> + // recurse left child until finding a MCSymbolRefExpr
>> + switch (E->getKind()) {
>> + case MCExpr::SymbolRef:
>> + return cast<MCSymbolRefExpr>(E);
>> + case MCExpr::Binary:
>> + return FindLHSymExpr(cast<MCBinaryExpr>(E)->getLHS());
>> + default:
>> + return NULL;
>> + }
>> +}
>> +
>> +// FIXME: This routine assumes that a binary
>> +// expression will always result in a PCRel expression
>> +// In reality, its only true if one or more subexpressions
>> +// is itself a PCRel (i.e. "." in asm or some other pcrel construct)
>> +// but this is good enough for now.
>
> This comment makes me very, very nervous. LLVM isn't always compiling ARM code as PIC. Can you help me understand why this is indeed good enough for now?
The long story is quite convoluted, but the simple answer is that when
the "immediate value" being loaded to a register via movw/movt is
itself an expression that evaluates to a PCrelative value,
GNU as creates two different relocation values for the MOVW/MOVT. This
patch is to address that case.
It just so happens that some 99.999% of such addresses, the
expressions are of the form:
movw r0, :lower16:Foo-(Bar+8)
movt r0, :upper16:Foo-(Bar+8)
For such expressions, GNU as creates two different relocations with
the reloc tags
R_ARM_MOVW_PREL_NC and R_ARM_MOVT_PREL,
In normal cases where the expression is not a pcrel value, GNU as
creates R_ARM_MOVT_ABS and R_ARM_MOVW_ABS_NC
The reason why that hack routine works is because in nearly all cases,
the expression winds up being a highly constrained Binary expression -
LLVM also constrains this because MCValue has room for exactly two
symbols (but in reality, the pcrel expression can be hairy beasts like
Foo+Bar-Baz + 8 etc... but llvm doesn't seem to support this directly)
>
>> +static bool EvaluateAsPCRel(const MCExpr *Expr) {
>> + switch (Expr->getKind()) {
>> + case MCExpr::SymbolRef: return false;
>> + case MCExpr::Binary: return true;
>> + default: assert(0 && "Unexpected expression type");
>> + }
>> +}
>> +
>> uint32_t ARMMCCodeEmitter::
>> getMovtImmOpValue(const MCInst &MI, unsigned OpIdx,
>> SmallVectorImpl<MCFixup> &Fixups) const {
>> @@ -635,18 +661,27 @@
>> if (MO.isImm()) {
>> return static_cast<unsigned>(MO.getImm());
>> } else if (const MCSymbolRefExpr *Expr =
>> - dyn_cast<MCSymbolRefExpr>(MO.getExpr())) {
>> + FindLHSymExpr(MO.getExpr())) {
>> + // FIXME: :lower16: and :upper16: should be applicable to
>> + // to whole expression, not just symbolrefs
>> + // Until that change takes place, this hack is required to
>> + // generate working code.
>> + const MCExpr *OrigExpr = MO.getExpr();
>> MCFixupKind Kind;
>> switch (Expr->getKind()) {
>> default: assert(0 && "Unsupported ARMFixup");
>> case MCSymbolRefExpr::VK_ARM_HI16:
>> Kind = MCFixupKind(ARM::fixup_arm_movt_hi16);
>> + if (EvaluateAsPCRel(OrigExpr))
>> + Kind = MCFixupKind(ARM::fixup_arm_movt_hi16_pcrel);
>> break;
>> case MCSymbolRefExpr::VK_ARM_LO16:
>> Kind = MCFixupKind(ARM::fixup_arm_movw_lo16);
>> + if (EvaluateAsPCRel(OrigExpr))
>> + Kind = MCFixupKind(ARM::fixup_arm_movw_lo16_pcrel);
>> break;
>> }
>> - Fixups.push_back(MCFixup::Create(0, Expr, Kind));
>> + Fixups.push_back(MCFixup::Create(0, OrigExpr, Kind));
>> return 0;
>> };
>> llvm_unreachable("Unsupported MCExpr type in MCOperand!");
>>
>> Modified: llvm/trunk/test/MC/ARM/elf-movt.s
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/MC/ARM/elf-movt.s?rev=123294&r1=123293&r2=123294&view=diff
>> ==============================================================================
>> --- llvm/trunk/test/MC/ARM/elf-movt.s (original)
>> +++ llvm/trunk/test/MC/ARM/elf-movt.s Tue Jan 11 18:19:25 2011
>> @@ -1,4 +1,6 @@
>> @ RUN: llvm-mc %s -triple=armv7-linux-gnueabi | FileCheck -check-prefix=ASM %s
>> +@ RUN: llvm-mc %s -triple=armv7-linux-gnueabi -filetype=obj -o - | \
>> +@ RUN: elf-dump --dump-section-data | FileCheck -check-prefix=OBJ %s
>> .syntax unified
>> .text
>> .globl barf
>> @@ -12,3 +14,26 @@
>> @ ASM: movw r0, :lower16:GOT-(.LPC0_2+8)
>> @ ASM-NEXT: movt r0, :upper16:GOT-(.LPC0_2+16)
>>
>> +@@ make sure that the text section fixups are sane too
>> +@ OBJ: '.text'
>> +@ OBJ-NEXT: 'sh_type', 0x00000001
>> +@ OBJ-NEXT: 'sh_flags', 0x00000006
>> +@ OBJ-NEXT: 'sh_addr', 0x00000000
>> +@ OBJ-NEXT: 'sh_offset', 0x00000034
>> +@ OBJ-NEXT: 'sh_size', 0x00000008
>> +@ OBJ-NEXT: 'sh_link', 0x00000000
>> +@ OBJ-NEXT: 'sh_info', 0x00000000
>> +@ OBJ-NEXT: 'sh_addralign', 0x00000004
>> +@ OBJ-NEXT: 'sh_entsize', 0x00000000
>> +@ OBJ-NEXT: '_section_data', 'f00f0fe3 ec0f4fe3'
>> +
>> +@ OBJ: Relocation 0x00000000
>> +@ OBJ-NEXT: 'r_offset', 0x00000000
>> +@ OBJ-NEXT: 'r_sym'
>> +@ OBJ-NEXT: 'r_type', 0x0000002d
>> +
>> +@ OBJ: Relocation 0x00000001
>> +@ OBJ-NEXT: 'r_offset', 0x00000004
>> +@ OBJ-NEXT: 'r_sym'
>> +@ OBJ-NEXT: 'r_type', 0x0000002e
>> +
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
More information about the llvm-commits
mailing list