tile: Fix up corner cases with signed relocations
Commit Message
Some types of relocations technically need to be signed rather than
unsigned: in particular ones that are used with moveli or movei,
or for jump and branch. This is almost never a problem. Jump and
branch opcodes are pretty much uniformly resolved by the static linker
(unless you omit -fpic for a shared library, which is not recommended).
The moveli and movei opcodes that need to be sign-extended generally
are for positive displacements, like the construction of the address of
main() from _start(). However, tst-pie1 ends up with main below _start
(in a different module) and the test failed due to signedness issues in
relocation handling.
This commit treats the value as signed when shifting (to preserve the
high bit) and also sign-extends the value generated from the updated
bundle when comparing with the desired bundle, which we do to make sure
no overflow occurred. As a result, the tst-pie1 test now passes.
---
sysdeps/tile/dl-machine.h | 40 ++++++++++++++++++++++------------------
1 files changed, 22 insertions(+), 18 deletions(-)
This is the change I wanted to push before 2.20 froze. It turns out
not to be a regression, so arguably the conservative thing may to wait
until after 2.21 opens. I've done reasonable internal testing (and
will have more results by Saturday morning after a full rebuild and
test suite run) but could easily be swayed one way or the other.
Comments
Chris Metcalf <cmetcalf@tilera.com> writes:
> @@ -686,13 +686,17 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
> tile_bundle_bits *p = (tile_bundle_bits *) reloc_addr;
> tile_bundle_bits bits = *p;
>
> -#define MUNGE(func) do { \
> +#define MUNGE_SIGNED(func, length) do { \
> bits = ((bits & ~create_##func (-1)) | create_##func (value)); \
> - if (get_##func (bits) != value) \
> + ElfW(Addr) result = (long) get_##func (bits) \
> + << (__WORDSIZE - length) >> (__WORDSIZE - length); \
Left shifting a negative value has undefined value.
Andreas.
On 9/5/2014 4:33 PM, Andreas Schwab wrote:
> Chris Metcalf <cmetcalf@tilera.com> writes:
>
>> @@ -686,13 +686,17 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
>> tile_bundle_bits *p = (tile_bundle_bits *) reloc_addr;
>> tile_bundle_bits bits = *p;
>>
>> -#define MUNGE(func) do { \
>> +#define MUNGE_SIGNED(func, length) do { \
>> bits = ((bits & ~create_##func (-1)) | create_##func (value)); \
>> - if (get_##func (bits) != value) \
>> + ElfW(Addr) result = (long) get_##func (bits) \
>> + << (__WORDSIZE - length) >> (__WORDSIZE - length); \
> Left shifting a negative value has undefined value.
The shift is always a non-negative value less than __WORDSIZE here, by intention. Are you seeing something I'm missing?
Chris Metcalf <cmetcalf@tilera.com> writes:
> On 9/5/2014 4:33 PM, Andreas Schwab wrote:
>> Chris Metcalf <cmetcalf@tilera.com> writes:
>>
>>> @@ -686,13 +686,17 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
>>> tile_bundle_bits *p = (tile_bundle_bits *) reloc_addr;
>>> tile_bundle_bits bits = *p;
>>> -#define MUNGE(func) do {
>>> \
>>> +#define MUNGE_SIGNED(func, length) do { \
>>> bits = ((bits & ~create_##func (-1)) | create_##func (value)); \
>>> - if (get_##func (bits) != value) \
>>> + ElfW(Addr) result = (long) get_##func (bits) \
>>> + << (__WORDSIZE - length) >> (__WORDSIZE - length); \
>> Left shifting a negative value has undefined value.
>
> The shift is always a non-negative value less than __WORDSIZE here, by intention. Are you seeing something I'm missing?
I'm talking about the value, not the shift amount.
Andreas.
On Fri, Sep 05, 2014 at 11:40:20PM +0200, Andreas Schwab wrote:
> Chris Metcalf <cmetcalf@tilera.com> writes:
>
> > On 9/5/2014 4:33 PM, Andreas Schwab wrote:
> >> Chris Metcalf <cmetcalf@tilera.com> writes:
> >>
> >>> @@ -686,13 +686,17 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
> >>> tile_bundle_bits *p = (tile_bundle_bits *) reloc_addr;
> >>> tile_bundle_bits bits = *p;
> >>> -#define MUNGE(func) do {
> >>> \
> >>> +#define MUNGE_SIGNED(func, length) do { \
> >>> bits = ((bits & ~create_##func (-1)) | create_##func (value)); \
> >>> - if (get_##func (bits) != value) \
> >>> + ElfW(Addr) result = (long) get_##func (bits) \
> >>> + << (__WORDSIZE - length) >> (__WORDSIZE - length); \
> >> Left shifting a negative value has undefined value.
> >
> > The shift is always a non-negative value less than __WORDSIZE
> > here, by intention. Are you seeing something I'm missing?
>
> I'm talking about the value, not the shift amount.
In general, this issue should always be solvable, in cases where
overflow won't happen, by replacing a<<b with a*(1<<b).
Rich
On 9/5/2014 5:40 PM, Andreas Schwab wrote:
>> On 9/5/2014 4:33 PM, Andreas Schwab wrote:
>>> Chris Metcalf <cmetcalf@tilera.com> writes:
>>>
>>>> @@ -686,13 +686,17 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
>>>> tile_bundle_bits *p = (tile_bundle_bits *) reloc_addr;
>>>> tile_bundle_bits bits = *p;
>>>> -#define MUNGE(func) do {
>>>> \
>>>> +#define MUNGE_SIGNED(func, length) do { \
>>>> bits = ((bits & ~create_##func (-1)) | create_##func (value)); \
>>>> - if (get_##func (bits) != value) \
>>>> + ElfW(Addr) result = (long) get_##func (bits) \
>>>> + << (__WORDSIZE - length) >> (__WORDSIZE - length); \
>>> Left shifting a negative value has undefined value.
>>>
> I'm talking about the value, not the shift amount.
Right, sorry. On tile, the compiler will always generate a "shli" for shifting an unknown value by a known constant, and shli is defined with unsigned semantics on tile, so in practice this code generates the correct value.
That said, you're right that we're better off to avoid relying on undefined behavior, so, thanks. I've changed that part of the code to look like:
ElfW(Addr) result = get_##func (bits); \
int signbits = __WORDSIZE - length; \
result = (long) (result << signbits) >> signbits; \
I'll go ahead and push the commit with that change.
On Sat, 6 Sep 2014, Chris Metcalf wrote:
> Right, sorry. On tile, the compiler will always generate a "shli" for
> shifting an unknown value by a known constant, and shli is defined with
> unsigned semantics on tile, so in practice this code generates the correct
> value.
In any case, the GNU C language defines signed shifts (as long as the
shift amount is >= 0 and < width of type), although it may still be useful
to avoid the cases that are outside what ISO C defines.
On Mon, Sep 08, 2014 at 03:20:59PM +0000, Joseph S. Myers wrote:
> On Sat, 6 Sep 2014, Chris Metcalf wrote:
>
> > Right, sorry. On tile, the compiler will always generate a "shli" for
> > shifting an unknown value by a known constant, and shli is defined with
> > unsigned semantics on tile, so in practice this code generates the correct
> > value.
>
> In any case, the GNU C language defines signed shifts (as long as the
> shift amount is >= 0 and < width of type), although it may still be useful
> to avoid the cases that are outside what ISO C defines.
I don't think this is true. For example, in many versions of GCC, this
is (rightfully!) an infinite loop:
int i;
for (i=1; i>0; i<<=1);
On the other hand, defining a<<b when a is negative but a*(2**b) does
not overflow makes sense, and it's frustrating that the C standard
doesn't already define this (but see my workaround, i.e. just using
*(1<<b) or similar). But defining a<<b when it would overflow is
inconsistent and unreasonable.
Rich
On Mon, 8 Sep 2014, Rich Felker wrote:
> > In any case, the GNU C language defines signed shifts (as long as the
> > shift amount is >= 0 and < width of type), although it may still be useful
> > to avoid the cases that are outside what ISO C defines.
>
> I don't think this is true. For example, in many versions of GCC, this
> is (rightfully!) an infinite loop:
>
> int i;
> for (i=1; i>0; i<<=1);
That would be a bug (I fixed such a bug ten years ago, PR 7284); please
report it if present in trunk. Signed shifts are documented in
implement-c.texi; C90 makes some things implementation-defined even when
C99/C11 make them undefined.
On Mon, Sep 08, 2014 at 03:34:18PM +0000, Joseph S. Myers wrote:
> On Mon, 8 Sep 2014, Rich Felker wrote:
>
> > > In any case, the GNU C language defines signed shifts (as long as the
> > > shift amount is >= 0 and < width of type), although it may still be useful
> > > to avoid the cases that are outside what ISO C defines.
> >
> > I don't think this is true. For example, in many versions of GCC, this
> > is (rightfully!) an infinite loop:
> >
> > int i;
> > for (i=1; i>0; i<<=1);
>
> That would be a bug (I fixed such a bug ten years ago, PR 7284); please
> report it if present in trunk. Signed shifts are documented in
> implement-c.texi; C90 makes some things implementation-defined even when
> C99/C11 make them undefined.
OK, yet another reason to avoid << operator: GCC intentionally fails
to optimize it right. Thankfully *(1<<y) solves that too.
As for versions where the above loop condition gets optimized out, I
don't recall which ones, but I do recall seeing it (broken configure
scripts checking the range of time_t going into an infinite loop) in
the past 3 or 4 years. But it's very possible that the users who
experienced it were using a much older version of GCC such as 3.4,
4.2, or 4.4.
Rich
@@ -657,7 +657,7 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
value += 0x8000;
#endif
- value >>= h->right_shift;
+ value = ((long) value) >> h->right_shift;
switch (h->byte_size)
{
@@ -686,13 +686,17 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
tile_bundle_bits *p = (tile_bundle_bits *) reloc_addr;
tile_bundle_bits bits = *p;
-#define MUNGE(func) do { \
+#define MUNGE_SIGNED(func, length) do { \
bits = ((bits & ~create_##func (-1)) | create_##func (value)); \
- if (get_##func (bits) != value) \
+ ElfW(Addr) result = (long) get_##func (bits) \
+ << (__WORDSIZE - length) >> (__WORDSIZE - length); \
+ if (result != value) \
_dl_signal_error (0, map->l_name, NULL, \
"relocation value too large for " #func); \
} while (0)
+#define MUNGE(func) MUNGE_SIGNED(func, __WORDSIZE)
+
#define MUNGE_NOCHECK(func) \
bits = ((bits & ~create_##func (-1)) | create_##func (value))
@@ -700,23 +704,23 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
{
#ifdef __tilegx__
case R_TILEGX_BROFF_X1:
- MUNGE (BrOff_X1);
+ MUNGE_SIGNED (BrOff_X1, 17);
break;
case R_TILEGX_JUMPOFF_X1:
case R_TILEGX_JUMPOFF_X1_PLT:
- MUNGE (JumpOff_X1);
+ MUNGE_SIGNED (JumpOff_X1, 27);
break;
case R_TILEGX_IMM8_X0:
- MUNGE (Imm8_X0);
+ MUNGE_SIGNED (Imm8_X0, 8);
break;
case R_TILEGX_IMM8_Y0:
- MUNGE (Imm8_Y0);
+ MUNGE_SIGNED (Imm8_Y0, 8);
break;
case R_TILEGX_IMM8_X1:
- MUNGE (Imm8_X1);
+ MUNGE_SIGNED (Imm8_X1, 8);
break;
case R_TILEGX_IMM8_Y1:
- MUNGE (Imm8_Y1);
+ MUNGE_SIGNED (Imm8_Y1, 8);
break;
case R_TILEGX_MT_IMM14_X1:
MUNGE (MT_Imm14_X1);
@@ -746,7 +750,7 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
case R_TILEGX_IMM16_X0_HW1_LAST_TLS_GD:
case R_TILEGX_IMM16_X0_HW0_LAST_TLS_IE:
case R_TILEGX_IMM16_X0_HW1_LAST_TLS_IE:
- MUNGE (Imm16_X0);
+ MUNGE_SIGNED (Imm16_X0, 16);
break;
case R_TILEGX_IMM16_X1_HW0:
case R_TILEGX_IMM16_X1_HW1:
@@ -770,7 +774,7 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
case R_TILEGX_IMM16_X1_HW1_LAST_TLS_GD:
case R_TILEGX_IMM16_X1_HW0_LAST_TLS_IE:
case R_TILEGX_IMM16_X1_HW1_LAST_TLS_IE:
- MUNGE (Imm16_X1);
+ MUNGE_SIGNED (Imm16_X1, 16);
break;
case R_TILEGX_MMSTART_X0:
MUNGE (BFStart_X0);
@@ -792,23 +796,23 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
break;
#else
case R_TILEPRO_BROFF_X1:
- MUNGE (BrOff_X1);
+ MUNGE_SIGNED (BrOff_X1, 17);
break;
case R_TILEPRO_JOFFLONG_X1:
case R_TILEPRO_JOFFLONG_X1_PLT:
MUNGE_NOCHECK (JOffLong_X1); /* holds full 32-bit value */
break;
case R_TILEPRO_IMM8_X0:
- MUNGE (Imm8_X0);
+ MUNGE_SIGNED (Imm8_X0, 8);
break;
case R_TILEPRO_IMM8_Y0:
- MUNGE (Imm8_Y0);
+ MUNGE_SIGNED (Imm8_Y0, 8);
break;
case R_TILEPRO_IMM8_X1:
- MUNGE (Imm8_X1);
+ MUNGE_SIGNED (Imm8_X1, 8);
break;
case R_TILEPRO_IMM8_Y1:
- MUNGE (Imm8_Y1);
+ MUNGE_SIGNED (Imm8_Y1, 8);
break;
case R_TILEPRO_MT_IMM15_X1:
MUNGE (MT_Imm15_X1);
@@ -834,7 +838,7 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
case R_TILEPRO_IMM16_X0_PCREL:
case R_TILEPRO_IMM16_X0_TLS_GD:
case R_TILEPRO_IMM16_X0_TLS_IE:
- MUNGE (Imm16_X0);
+ MUNGE_SIGNED (Imm16_X0, 16);
break;
case R_TILEPRO_IMM16_X1_LO:
case R_TILEPRO_IMM16_X1_HI:
@@ -854,7 +858,7 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
case R_TILEPRO_IMM16_X1_PCREL:
case R_TILEPRO_IMM16_X1_TLS_GD:
case R_TILEPRO_IMM16_X1_TLS_IE:
- MUNGE (Imm16_X1);
+ MUNGE_SIGNED (Imm16_X1, 16);
break;
case R_TILEPRO_MMSTART_X0:
MUNGE (MMStart_X0);