[PATCH] D30699: [ELF] - Stop producing broken output for R_386_GOT32X relocation.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/Target.cpp:369
+RelExpr X86TargetInfo::getRelExpr(uint32_t Type, const SymbolBody &S,
+                                  const uint8_t *Loc) const {
+  // R_386_GOT32X usually calculated as G + A - GOT.
----------------
ruiu wrote:
> Is there any way to not add a new parameter to this function? This part of code has grown organically to the point that that's hard to understand. Needs simplifying.
I tried playing with code, but
problem is that this relocation has 2 ways of calculations which depends on was base register used or not.
in this method we return the expression used for calculation. And it depends on specific target instruction.

One way to avoid adding parameter here is to introduce new expression type like
R_GOT_OR_GOTOFFSET for this and postpone instruction checks. 

But it does not work good.
At some point (in getRelocTargetVA()) we still need to know how to calculate the final value,
that requires the same target specific logic (scanning ModRM), what does not fit there because getRelocTargetVA() is completely target independent.

Also It does not look that introducing one more relocation expression type really worth that. 
It involves additional code changes for places where expressions are checked, though at fact
we can use already existent ones and evaluate the relocation expression type early, like I do.

It seems for me that method is a best place for this logic.


================
Comment at: ELF/Target.cpp:377-388
+    // ModR/M byte has form XX YYY ZZZ, where
+    // YYY is MODRM.reg(register 2), ZZZ is MODRM.rm(register 1).
+    // XX has different meanings:
+    // 00: The operand's memory address is in reg1.
+    // 01: The operand's memory address is reg1 + a byte-sized displacement.
+    // 10: The operand's memory address is reg1 + a word-sized displacement.
+    // 11: The operand is reg1 itself.
----------------
ruiu wrote:
> I don't think you need to repeat the x86 instruction format here. Instead you want to describe it at high level. If I understand correctly, you are checking if the instruction the relocation is pointing to has %ebp as its operand, right?
No, I am checking that effective address is "disp32", and not "[SOME REGISTER] + disp32" (The disp32 nomenclature denotes a 32-bit displacement that follows the ModR/M byte (or the SIB byte if one is present) and that is added to the index.)
see "Table 2-2. 32-Bit Addressing Forms with the ModR/M Byte" (2-6 Vol. 2A).

Examples of baseless instructions:
movl ifunc at GOT, %eax (8b 05 00 00 00 00)
movl ifunc at GOT, %ebx (8b 1d 00 00 00 00)
05 == 00101b, ModRM = 0x5 & 0xc7 = 0x5
1d == 11101b, ModRM = 0x1d & 0xc7 = 0x5
I am checking that ModR/M == 00101 == 0x5, that corresponds to effective address = disp32, so instruction
pointed by relocation has no base register and uses disp32 value as is. 
After link it contains got entry address as value (and not an offset), what works only when no-PIC. And for PIC
case instruction has to have base register for keeping GOT address, relocation resolves to GOT offset then.


================
Comment at: ELF/Target.cpp:394-395
+              " against '" + S.getName() +
+              "' without base register can not be "
+              "used when PIC enabled");
+      return R_GOT;
----------------
ruiu wrote:
> Concatenate the strings.
Done.


https://reviews.llvm.org/D30699