Re: [Tinycc-devel] [PATCH] use RELA relocations properly for R_DATA_PTR

[Michael Matz]

Hello Edmund,

On Tue, 17 Feb 2015, Edmund Grimley Evans wrote:

> With TCC the addend is zero! Apparently TCC is putting the offset is
> in the data section:
>
> readelf -x .data t1.gcc.o
>  0x00000000 00000000 00000000                   ........
>
> readelf -x .data t1.tcc.o
>  0x00000000 10000000 00000000                   ........
>
> Now this isn't how RELA relocations are normally described as working,
> though I'm not sure that it's wrong.

It's strictly wrong, but harmless, because, as you noticed, all linkers OR 
in the value at the reloc place.  It's one of those things that always 
wants fixing but never really reach the top of TODO lists :)  As it 
reached yours, all the better.

> I haven't found a document that says you have to ignore what value is 
> "in the place" with a RELA relocation,

Actually, the x86-64 psABI is explicit.  The formulas for calculating 
reloc values only contain one 'A' (addend) and specify that the addend is 
in r_addend of the reloc, so the "addend" at the place must be ignored. 
But alas, real world is more forgiving :)

> Proposed commit message:
>
> Use RELA relocations properly for R_DATA_PTR on x86_64.
>
> libtcc.c: Add greloca, a generalisation of greloc that takes an addend.
> tcc.h: Add greloca and put_elf_reloca.
> tccelf.c: Add put_elf_reloca, a generalisation of put_elf_reloc.
> tccgen.c: On x86_64, use greloca instead of greloc in init_putv.

The patch is fine, please commit.  I have one remark:

-            if (vtop->r & VT_SYM) {
+#ifdef TCC_TARGET_X86_64
+            if (vtop->r & VT_SYM)
+                greloca(sec, vtop->sym, c, R_DATA_PTR, vtop->c.ptr_offset);
+            else
+                *(addr_t *)ptr |= (vtop->c.ptr_offset & bit_mask) << bit_pos;
+#else
+            if (vtop->r & VT_SYM)
                 greloc(sec, vtop->sym, c, R_DATA_PTR);
-            }
             *(addr_t *)ptr |= (vtop->c.ptr_offset & bit_mask) << bit_pos;
+#endif

Strictly speaking the addend should be
  (vtop->c.ptr_offset & bit_mask) << bit_pos;
This should make no difference for VT_SYM vtops, but perhaps an assert 
would be better (bit_pos == 0).  OTOH this routine has other checking 
problems (e.g. it doesn't give an error if the value doesn't fit e.g. a 
VT_BYTE place), so adding just one assert for this case seems 
inconsistent.

Oh, and one other thing: if you could include (smallish) patches inline at 
the end of mail, it makes quoting parts of them for review easier; of 
course only if your mailer doesn't mangle patches added inline.


Ciao,
Michael.