Bug 84184 - gcc generates wrong relocations with negative offsets in struct arrays
Summary: gcc generates wrong relocations with negative offsets in struct arrays
Status: RESOLVED DUPLICATE of bug 86984
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 7.2.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-02-02 17:38 UTC by Sergei Trofimovich
Modified: 2018-08-20 15:05 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-02-02 00:00:00


Attachments
reloc-bug.c (731 bytes, text/x-csrc)
2018-02-02 17:38 UTC, Sergei Trofimovich
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sergei Trofimovich 2018-02-02 17:38:49 UTC
Created attachment 43327 [details]
reloc-bug.c

It's a trimmed-down version of linux kernel miscompilation on ia64:
    https://bugs.gentoo.org/518130

Minimal reproducer:

// cat reloc-bug.c
#include <stdio.h>

typedef unsigned long long u64;

struct s { u64 a; };
struct s glo_s[] = {
    { 100, },
// glo_s_middle_hidden:
// ; Injected as LDFLAGS="-Wl,--defsym=glo_s_middle=glo_s+8 -Wl,--defsym=glo_s_middle_hidden=glo_s+8"
    { 700, },
    { 800, },
};

extern char glo_s_middle_hidden[] __attribute__((visibility("hidden")));

static u64 __attribute__((noinline)) val_s_hidden(void) {
    const struct s * m = (const struct s *)glo_s_middle_hidden;
    return m[-1].a;
}

int main() {
    printf ("val/hidden = %lld\n", val_s_hidden());
}

+ ia64-unknown-linux-gnu-gcc-7.2.0 -c -O2 reloc-bug.c -o reloc-bug.o
+ ia64-unknown-linux-gnu-gcc-7.2.0 -O2 reloc-bug.o -o reloc-bug -Wl,--defsym=glo_s_middle=glo_s+8 -Wl,--defsym=glo_s_middle_hidden=glo_s+8
+ ./reloc-bug
./mk.sh: line 12: 12418 Segmentation fault      ./reloc-bug

Note a few things:

Note 1: 
it's not trivial to write the code in standard C. I had to synthesise symbol with --defsym. I tried a few targets where this sample crashes instead of returning 100:

# fails
CC=sparc64-unknown-linux-gnu-gcc
CC=ia64-unknown-linux-gnu-gcc
CC=alpha-unknown-linux-gnu-gcc

# works
CC=sparc-unknown-linux-gnu-gcc
CC=x86_64-pc-linux-gnu-gcc
CC=aarch64-unknown-linux-gnu-gcc
CC=armv5tel-softfloat-linux-gnueabi-gcc
CC=hppa-unknown-linux-gnu-gcc
CC=hppa2.0-unknown-linux-gnu-gcc
CC=m68k-unknown-linux-gnu-gcc
CC=mips-unknown-linux-gnu-gcc
CC=nios2-unknown-linux-gnu-gcc
CC=powerpc-unknown-linux-gnu-gcc
CC=powerpc64-unknown-linux-gnu-gcc
CC=powerpc64le-unknown-linux-gnu-gcc
CC=s390x-unknown-linux-gnu-gcc
CC=sh4-unknown-linux-gnu-gcc

Note 2:

If we change
    struct s { u64 a; };
    struct s glo_s[] = {
to
    u64 glo_s[] = {
gcc will generate correct code.
Comment 1 Sergei Trofimovich 2018-02-02 17:50:24 UTC
Dumping 'val_s_hidden':

    extern char glo_s_middle_hidden[] __attribute__((visibility("hidden")));
    static u64 __attribute__((noinline)) val_s_hidden(void) {
        const struct s * m = (const struct s *)glo_s_middle_hidden;
        return m[-1].a;
    }

$ cat reloc-bug.c.227t.optimized

    ;; Function val_s_hidden (val_s_hidden, funcdef_no=23, decl_uid=2082, cgraph_uid=23, symbol_order=24) (executed once)

    __attribute__((noinline))
    val_s_hidden ()
    {
      u64 _2;

      <bb 2> [100.00%]:
      _2 = MEM[(const struct s *)&glo_s_middle_hidden + -8B].a;
      return _2;
    }

Still looks ok, right? I guess RTL is doing funny things here (+Eric).

$ cat reloc-bug.c.229r.expand

    ;;
    ;; Full RTL generated for this function:
    ;;
    (note 1 0 3 NOTE_INSN_DELETED)
    (note 3 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK)
    (note 2 3 5 2 NOTE_INSN_FUNCTION_BEG)
    (insn 5 2 6 2 (set (reg/f:DI 342)
            (symbol_ref:DI ("glo_s_middle_hidden") [flags 0x42]  <var_decl 0x7f03587b2510 glo_s_middle_hidden>)) "reloc-bug.c":17 -1
         (nil))
    (insn 6 5 7 2 (set (reg:DI 343)
            (reg/f:DI 342)) "reloc-bug.c":17 -1
         (nil))
    (insn 7 6 8 2 (set (reg:DI 345)
            (const_int 2305843009213693944 [0x1ffffffffffffff8])) "reloc-bug.c":17 -1
         (nil))
    ...

Surprisingly large amount of code is generated here:

$ cat reloc-bug.S

val_s_hidden:
        .prologue
        .body
        .mlx
        nop 0
        movl r14 = @gprel(glo_s_middle_hidden#)
        .mlx
        nop 0
        movl r16 = 2305843009213693945 ; 0x1ffffffffffffff9
        ;;
        .mlx
        add r14 = r1, r14
        movl r15 = 2305843009213693944 ; 0x1ffffffffffffff8
        .mlx
        nop 0
        movl r17 = 2305843009213693946 ; 0x1ffffffffffffffa
        ;;
        .mmi
        add r15 = r14, r15
        add r17 = r14, r17
        add r16 = r14, r16
        ;;
        .mmi
        ld1 r8 = [r15]
        ld1 r16 = [r16]
        nop 0
        ;;
        .mlx
        ld1 r15 = [r17]
        movl r17 = 2305843009213693947 ; 0x1ffffffffffffffb
        .mib
        nop 0
        shl r16 = r16, 8
        nop 0
        ;;
        .mmi
        add r17 = r14, r17
        or r16 = r8, r16
        shl r15 = r15, 16
        ;;
        .mlx
        ld1 r8 = [r17]
        movl r17 = 2305843009213693948 ; 0x1ffffffffffffffc
        .mmi
        or r15 = r16, r15
        ;;
        add r17 = r14, r17
        shl r8 = r8, 24
        ;;
        .mlx
        ld1 r16 = [r17]
        movl r17 = 2305843009213693949 ; 0x1ffffffffffffffd
        .mmi
        or r8 = r15, r8
        ;;
        add r17 = r14, r17
        shl r16 = r16, 32
        ;;
        .mlx
        ld1 r15 = [r17]
        movl r17 = 2305843009213693950 ; 0x1ffffffffffffffe
        .mmi
        or r16 = r8, r16
        ;;
        add r17 = r14, r17
        shl r15 = r15, 40
        ;;
        .mlx
        ld1 r8 = [r17]
        movl r17 = 2305843009213693951 ; 0x1fffffffffffffff
        .mmi
        or r15 = r16, r15
        ;;
        add r14 = r14, r17
        shl r8 = r8, 48
        ;;
        .mii
        ld1 r16 = [r14]
        or r15 = r15, r8
        ;;
        shl r8 = r16, 56
        ;;
        .mib
        nop 0
        or r8 = r15, r8
        br.ret.sptk.many b0
        .endp val_s_hidden#

I hoped to see here single load (something like that):

    mov <reg_rel> = @gprel64(glo_s_middle_hidden#)

    add  <reg> = gp, <reg_rel>

    ld8 r8 = [<reg>]

I see 2 issues here:
- code is invalid
- large amount of code
Comment 2 Sergei Trofimovich 2018-02-02 19:23:29 UTC
> - large amount of code

Jason pointed out the blowup happens due to byte-level reads (caused by 'char*' -> 'u64*' required alignment increase) thus it's expected. Only correctness issue is left.
Comment 3 Eric Botcazou 2018-02-02 20:05:00 UTC
Calling this standard C is really questionable.  This goes awry on 64-bit RISC architectures with strict-alignment requirement because get_inner_reference returns a negative bit position and passes it down to the bitfield extraction machinery, which expects only non-negative bit positions.
Comment 4 Eric Botcazou 2018-02-02 20:08:30 UTC
I'd suggest fixing the code instead:

extern struct s glo_s_middle_hidden[] __attribute__((visibility("hidden")));

which makes it valid C and generates correct code.
Comment 5 Sergei Trofimovich 2018-02-02 20:35:39 UTC
Good suggestion! I will do it.

Could gcc generate warning without much of additional effort (or even better an error) when it knows it is about to generate broken code?

For this code I guess the pattern is <symbol> + <negative_literal>.
Comment 6 Eric Botcazou 2018-02-02 21:10:32 UTC
> Could gcc generate warning without much of additional effort (or even better
> an error) when it knows it is about to generate broken code?

Generating broken code from invalid C is perfectly OK, but we could indeed warn.

> For this code I guess the pattern is <symbol> + <negative_literal>.

The key is really the misalignment, which forces the bitfield path.  Other paths in the compiler handle the negative literal just fine.
Comment 7 Eric Botcazou 2018-02-02 21:20:09 UTC
Investigating about the warning.
Comment 8 Eric Botcazou 2018-02-02 21:20:43 UTC
Recategorizing.
Comment 9 Sergei Trofimovich 2018-02-02 22:19:24 UTC
Aha, makes sense! Proposed kernel tweak upstream as: https://lkml.org/lkml/2018/2/2/914
Comment 10 Sergei Trofimovich 2018-02-02 23:45:43 UTC
Oh, I have forgot to ask another question:

In attached reloc-bug.c there is seemingly two functionally identical samples:

  extern char glo_u64_middle_hidden[] __attribute__((visibility("hidden")));
  static u64 __attribute__((noinline)) val_u64_hidden(void) {
    const u64 * m = (const u64 *)glo_u64_middle_hidden;
    return m[-1];
  }

and

  extern char glo_s_middle_hidden[] __attribute__((visibility("hidden")));
  struct s { u64 a; };
  static u64 __attribute__((noinline)) val_s_hidden(void) {
    const struct s * m = (const struct s *)glo_s_middle_hidden;
    return m[-1].a;
  }

Why those are handled differently? First looks like it works, second does not. It was my main signal to file a bug against gcc as asymmetry looked fishy.

$ ./reloc-bug
val/global = 100
val/hidden = 100
Segmentation fault
Comment 11 Eric Botcazou 2018-02-03 11:42:57 UTC
> Why those are handled differently? First looks like it works, second does
> not. It was my main signal to file a bug against gcc as asymmetry looked
> fishy.

Because the problematic bitfield path is only used for fields in structures, i.e. misaligned integers are handled by another, simpler path.
Comment 12 Ulya 2018-08-04 19:12:01 UTC
(In reply to Eric Botcazou from comment #11)
> > Why those are handled differently? First looks like it works, second does
> > not. It was my main signal to file a bug against gcc as asymmetry looked
> > fishy.
> 
> Because the problematic bitfield path is only used for fields in structures,
> i.e. misaligned integers are handled by another, simpler path.

More details on the problematic path for this simple example:

    extern char __some_table[] __attribute__((visibility("hidden")));                                                                                                         
    struct s { long v; };                                                                                                                                                     
    long end(void) { return ((struct s *)__some_table)[-1].v; }                                                                                                               

1. The problematic path makes an illegal signed-to-unsigned integer conversion in expand_expr_real_1 (expr.c) when passing the signed 'bitnum' variable, with value -64, as the 3rd param of extract_bit_field (expmed.c), with value 18446744073709551552.

2. The conversion itself doesn't spoil the value (meaning that the bits of 'bitnum' are not changed), and the value is passed on to extract_bit_field_1, extract_integral_bit_field and extract_fixed_bit_field unharmed.

3. Finally, 'bitnum' is passed as the 5th param 'bitpos' to extract_split_bit_field, where it gets involved in unsigned integer arithmetics and bad things start to happen:


static rtx                                                                                                                                                                
extract_split_bit_field (rtx op0, opt_scalar_int_mode op0_mode,                                                                                                           
                         unsigned HOST_WIDE_INT bitsize,                                                                                                                  
                         unsigned HOST_WIDE_INT bitpos, int unsignedp,                                                                                                    
                         bool reverse)                                                                                                                                    
{                                                                                                                                                                         
  unsigned int unit;                                                                                                                                                      
  unsigned int bitsdone = 0;                                                                                                                                              
  // ...                                                                                                                                                                          
  while (bitsdone < bitsize)                                                                                                                                              
    {                                                                                                                                                                     
      unsigned HOST_WIDE_INT thissize;                                                                                                                                    
      rtx part;                                                                                                                                                           
      unsigned HOST_WIDE_INT thispos;                                                                                                                                     
      unsigned HOST_WIDE_INT offset;                                                                                                                                      
                                                                                                                                                                          
      offset = (bitpos + bitsdone) / unit;      // <=== BAD THING 1                                                                                                                               
      thispos = (bitpos + bitsdone) % unit;     // <=== BAD THING 2                                                                                                        
  // ...


Regardless of whether GCC wants to handle this example or not, implicit signed-to-unsigned conversion looks wrong to me. Even an assertion failure from the compiler is better than this silent code corruption. Furthermore, can we even guess all the possible cases when the problematic path is taken?


Full backtrace:

Breakpoint 15, extract_split_bit_field (op0=0x7ffff6cb7c60, op0_mode=..., bitsize=64, bitpos=18446744073709551552, unsignedp=0, reverse=false)                           
    at ../../gcc/gcc/expmed.c:2266
2266          thispos = (bitpos + bitsdone) % unit;
(gdb) bt
#0  extract_split_bit_field (op0=0x7ffff6cb7c60, op0_mode=..., bitsize=64, bitpos=18446744073709551552, unsignedp=0, reverse=false) at ../../gcc/gcc/expmed.c:2266       
#1  0x0000000000aafff7 in extract_fixed_bit_field (tmode=E_DImode, op0=0x7ffff6cb7c60, op0_mode=..., bitsize=64, bitnum=18446744073709551552, target=0x7ffff6cb7bb8,     
    unsignedp=0, reverse=false) at ../../gcc/gcc/expmed.c:2125
#2  0x0000000000aaf793 in extract_integral_bit_field (op0=0x7ffff6cb7c60, op0_mode=..., bitsize=64, bitnum=18446744073709551552, unsignedp=0, target=0x7ffff6cb7bb8,     
    mode=E_DImode, tmode=E_DImode, reverse=false, fallback_p=true) at ../../gcc/gcc/expmed.c:2016                                                                        
#3  0x0000000000aaeb74 in extract_bit_field_1 (str_rtx=0x7ffff6cb7c60, bitsize=..., bitnum=..., unsignedp=0, target=0x7ffff6cb7bb8, mode=E_DImode, tmode=E_DImode,       
    reverse=false, fallback_p=true, alt_rtl=0x0) at ../../gcc/gcc/expmed.c:1827
#4  0x0000000000aafe8c in extract_bit_field (str_rtx=0x7ffff6cb7c60, bitsize=..., bitnum=..., unsignedp=0, target=0x7ffff6cb7bb8, mode=E_DImode, tmode=E_DImode,         
    reverse=false, alt_rtl=0x0) at ../../gcc/gcc/expmed.c:2096
#5  0x0000000000aecaff in expand_expr_real_1 (exp=0x7ffff6ca6840, target=0x7ffff6cb7bb8, tmode=E_DImode, modifier=EXPAND_NORMAL, alt_rtl=0x0, inner_reference_p=false)   
    at ../../gcc/gcc/expr.c:10777
#6  0x0000000000adfef9 in expand_expr_real (exp=0x7ffff6ca6840, target=0x7ffff6cb7bb8, tmode=E_DImode, modifier=EXPAND_NORMAL, alt_rtl=0x0, inner_reference_p=false)     
    at ../../gcc/gcc/expr.c:8186
#7  0x0000000000ae799e in expand_expr_real_1 (exp=0x7ffff6b9f558, target=0x7ffff6cb7bb8, tmode=E_DImode, modifier=EXPAND_NORMAL, alt_rtl=0x0, inner_reference_p=false)   
    at ../../gcc/gcc/expr.c:9838
#8  0x0000000000adfef9 in expand_expr_real (exp=0x7ffff6b9f558, target=0x7ffff6cb7bb8, tmode=E_DImode, modifier=EXPAND_NORMAL, alt_rtl=0x0, inner_reference_p=false)     
    at ../../gcc/gcc/expr.c:8186
#9  0x000000000094d615 in expand_expr (exp=0x7ffff6b9f558, target=0x7ffff6cb7bb8, mode=E_DImode, modifier=EXPAND_NORMAL) at ../../gcc/gcc/expr.h:279                     
#10 0x00000000009590b0 in expand_return (retval=0x7ffff6c85f50) at ../../gcc/gcc/cfgexpand.c:3504                                                                        
#11 0x0000000000959507 in expand_gimple_stmt_1 (stmt=0x7ffff6cae000) at ../../gcc/gcc/cfgexpand.c:3607                                                                   
#12 0x0000000000959a87 in expand_gimple_stmt (stmt=0x7ffff6cae000) at ../../gcc/gcc/cfgexpand.c:3734                                                                     
#13 0x000000000096239c in expand_gimple_basic_block (bb=0x7ffff6c8b138, disable_tail_calls=false) at ../../gcc/gcc/cfgexpand.c:5769                                      
#14 0x0000000000963bd4 in (anonymous namespace)::pass_expand::execute (this=0x233f680, fun=0x7ffff6ca9000) at ../../gcc/gcc/cfgexpand.c:6372                             
#15 0x0000000000e4f77e in execute_one_pass (pass=0x233f680) at ../../gcc/gcc/passes.c:2446                                                                               
#16 0x0000000000e4fae3 in execute_pass_list_1 (pass=0x233f680) at ../../gcc/gcc/passes.c:2535                                                                            
#17 0x0000000000e4fb6c in execute_pass_list (fn=0x7ffff6ca9000, pass=0x233ba50) at ../../gcc/gcc/passes.c:2546                                                           
#18 0x00000000009b225b in cgraph_node::expand (this=0x7ffff6cab000) at ../../gcc/gcc/cgraphunit.c:2116                                                                   
#19 0x00000000009b28a0 in expand_all_functions () at ../../gcc/gcc/cgraphunit.c:2254
#20 0x00000000009b347d in symbol_table::compile (this=0x7ffff6b99000) at ../../gcc/gcc/cgraphunit.c:2605                                                                 
#21 0x00000000009b3714 in symbol_table::finalize_compilation_unit (this=0x7ffff6b99000) at ../../gcc/gcc/cgraphunit.c:2698                                               
#22 0x0000000000f9636c in compile_file () at ../../gcc/gcc/toplev.c:480
#23 0x0000000000f98d72 in do_compile () at ../../gcc/gcc/toplev.c:2161
#24 0x0000000000f99081 in toplev::main (this=0x7fffffffcf66, argc=17, argv=0x7fffffffd068) at ../../gcc/gcc/toplev.c:2296                                                
#25 0x0000000001928030 in main (argc=17, argv=0x7fffffffd068) at ../../gcc/gcc/main.c:39
Comment 13 Eric Botcazou 2018-08-20 15:05:10 UTC
.

*** This bug has been marked as a duplicate of bug 86984 ***