Message ID | 20180624115027.zjjyorpdpb32ekm6@localhost.localdomain |
---|---|
State | New |
Headers | show |
Series | [PR86257,i386/debug] Fix insn prefix in tls_global_dynamic_64_<mode> | expand |
> Hi, > > [ The analysis of this PR was done at https://stackoverflow.com/a/33557963 , > November 2015. ] > > This tls sequence: > ... > 0x00 .byte 0x66 > 0x01 leaq x@tlsgd(%rip),%rdi > 0x08 .word 0x6666 > 0x0a rex64 > 0x0b call __tls_get_addr@plt > ... > starts with an insn prefix, produced using a .byte. > > When using a .loc before the sequence, it's associated with the next assembly > instruction, which is the leaq, so the .loc will end up pointing inside the > sequence rather than to the start of the sequence. And when the linker > relaxes the sequence, the .loc may end up pointing inside an insn. This will > cause an executable containing such a misplaced .loc to crash when gdb > continues from the associated breakpoint. Hmm, I am not sure why .byte should be non-instruction and .data should be instruction, but I assume gas simply behaves this way. Don't we have also other cases wehre .byte is used to output instructions? Patch is OK (and probably should be backported after some soaking in mainline) Honza > > This patch fixes the problem by using data16 to generate the prefix. > > Bootstrapped and reg-tested on x86_64. > > OK for trunk? > > Thanks, > - Tom > > [i386/debug] Fix insn prefix in tls_global_dynamic_64_<mode> > > 2018-06-22 Tom de Vries <tdevries@suse.de> > > PR debug/86257 > * config/i386/i386.md (define_insn "*tls_global_dynamic_64_<mode>"): > Use data16 instead of .byte for insn prefix. > > * gcc.dg/pr86257.c: New test. > > --- > gcc/config/i386/i386.md | 13 ++++++++++++- > gcc/testsuite/gcc.dg/pr86257.c | 14 ++++++++++++++ > 2 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md > index eb77ef3c08f..6f2300057aa 100644 > --- a/gcc/config/i386/i386.md > +++ b/gcc/config/i386/i386.md > @@ -14733,7 +14733,18 @@ > "TARGET_64BIT" > { > if (!TARGET_X32) > - fputs (ASM_BYTE "0x66\n", asm_out_file); > + /* The .loc directive has effect for 'the immediately following assembly > + instruction'. So for a sequence: > + .loc f l > + .byte x > + insn1 > + the 'immediately following assembly instruction' is insn1. > + We want to emit an insn prefix here, but if we use .byte (as shown in > + 'ELF Handling For Thread-Local Storage'), a preceding .loc will point > + inside the insn sequence, rather than to the start. After relaxation > + of the sequence by the linker, the .loc might point inside an insn. > + Use data16 prefix instead, which doesn't have this problem. */ > + fputs ("\tdata16", asm_out_file); > output_asm_insn > ("lea{q}\t{%E1@tlsgd(%%rip), %%rdi|rdi, %E1@tlsgd[rip]}", operands); > if (TARGET_SUN_TLS || flag_plt || !HAVE_AS_IX86_TLS_GET_ADDR_GOT) > diff --git a/gcc/testsuite/gcc.dg/pr86257.c b/gcc/testsuite/gcc.dg/pr86257.c > new file mode 100644 > index 00000000000..3287c190d36 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/pr86257.c > @@ -0,0 +1,14 @@ > +/* { dg-require-effective-target fpic } */ > +/* { dg-require-effective-target tls } */ > +/* { dg-options "-g -fPIC" } */ > + > +__thread int i; > + > +void > +foo(void) > +{ > + i = 0; > +} > + > +/* { dg-final { scan-assembler "data16\[ \t\]*leaq" } } */ > +/* { dg-final { scan-assembler-not "\.byte\[ \t\]*0x66\n\[ \t\]*leaq" } } */
Hi, searching for other occurences I see: jan@skylake:~/trunk/gcc/config/i386> grep ASM_BYTE *md *.c i386.md: return ASM_BYTE "0x9e"; i386.md: fputs (ASM_BYTE "0x66\n", asm_out_file); i386.md: fputs (ASM_BYTE "0x66\n", asm_out_file); i386.c: fputs (ASM_BYTE "0x48, 0x8d, 0xa4, 0x24, 0x00, 0x00, 0x00, 0x00\n", i386.c: fputs (ASM_BYTE "0x8b, 0xff, 0x55, 0x8b, 0xec\n", asm_out_file); i386.c: fputs ("\n" ASM_BYTE "0xf2\n\t", file); i386.c: fputs ("\n" ASM_BYTE "0xf3\n\t", file); i386.c: fprintf (file, "1:" ASM_BYTE "0x0f, 0x1f, 0x44, 0x00, 0x00\n"); i386.c:#undef TARGET_ASM_BYTE_OP i386.c:#define TARGET_ASM_BYTE_OP ASM_BYTE Perhaps we want to add new macro like ASM_INSN_BYTE which is used to output such prefixes... Honza
On 06/24/2018 11:56 PM, Jan Hubicka wrote: >> Hi, >> >> [ The analysis of this PR was done at https://stackoverflow.com/a/33557963 , >> November 2015. ] >> >> This tls sequence: >> ... >> 0x00 .byte 0x66 >> 0x01 leaq x@tlsgd(%rip),%rdi >> 0x08 .word 0x6666 >> 0x0a rex64 >> 0x0b call __tls_get_addr@plt >> ... >> starts with an insn prefix, produced using a .byte. >> >> When using a .loc before the sequence, it's associated with the next assembly >> instruction, which is the leaq, so the .loc will end up pointing inside the >> sequence rather than to the start of the sequence. And when the linker >> relaxes the sequence, the .loc may end up pointing inside an insn. This will >> cause an executable containing such a misplaced .loc to crash when gdb >> continues from the associated breakpoint. > > Hmm, I am not sure why .byte should be non-instruction and .data should be instruction, > but I assume gas simply behaves this way. > Well, I suppose when encountering .byte/.value/.long/.quad, gas has no way of knowing whether it's dealing with data or instructions, even in the text section. An even if it would know it's dealing with instructions, it wouldn't know where an instruction begins or ends. So to me the behaviour seems reasonable. If we'd implemented something like this in gas: ... .insn .byte 0x66 .endinsn ... we could fix this more generically. Or maybe we'd want this, which allows us to express that the .byte is a prefix to an existing insn: ... .insn .byte 0x66 leaq x@tlsgd(%rip),%rdi .endinsn ... > Don't we have also other cases wehre .byte is used to output instructions? > > Patch is OK (and probably should be backported after some soaking in mainline) > Committed (after moving the testcase to gcc.target/i386). Thanks, - Tom > Honza >> >> This patch fixes the problem by using data16 to generate the prefix. >> >> Bootstrapped and reg-tested on x86_64. >> >> OK for trunk? >> >> Thanks, >> - Tom >> >> [i386/debug] Fix insn prefix in tls_global_dynamic_64_<mode> >> >> 2018-06-22 Tom de Vries <tdevries@suse.de> >> >> PR debug/86257 >> * config/i386/i386.md (define_insn "*tls_global_dynamic_64_<mode>"): >> Use data16 instead of .byte for insn prefix. >> >> * gcc.dg/pr86257.c: New test. >> >> --- >> gcc/config/i386/i386.md | 13 ++++++++++++- >> gcc/testsuite/gcc.dg/pr86257.c | 14 ++++++++++++++ >> 2 files changed, 26 insertions(+), 1 deletion(-) >> >> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md >> index eb77ef3c08f..6f2300057aa 100644 >> --- a/gcc/config/i386/i386.md >> +++ b/gcc/config/i386/i386.md >> @@ -14733,7 +14733,18 @@ >> "TARGET_64BIT" >> { >> if (!TARGET_X32) >> - fputs (ASM_BYTE "0x66\n", asm_out_file); >> + /* The .loc directive has effect for 'the immediately following assembly >> + instruction'. So for a sequence: >> + .loc f l >> + .byte x >> + insn1 >> + the 'immediately following assembly instruction' is insn1. >> + We want to emit an insn prefix here, but if we use .byte (as shown in >> + 'ELF Handling For Thread-Local Storage'), a preceding .loc will point >> + inside the insn sequence, rather than to the start. After relaxation >> + of the sequence by the linker, the .loc might point inside an insn. >> + Use data16 prefix instead, which doesn't have this problem. */ >> + fputs ("\tdata16", asm_out_file); >> output_asm_insn >> ("lea{q}\t{%E1@tlsgd(%%rip), %%rdi|rdi, %E1@tlsgd[rip]}", operands); >> if (TARGET_SUN_TLS || flag_plt || !HAVE_AS_IX86_TLS_GET_ADDR_GOT) >> diff --git a/gcc/testsuite/gcc.dg/pr86257.c b/gcc/testsuite/gcc.dg/pr86257.c >> new file mode 100644 >> index 00000000000..3287c190d36 >> --- /dev/null >> +++ b/gcc/testsuite/gcc.dg/pr86257.c >> @@ -0,0 +1,14 @@ >> +/* { dg-require-effective-target fpic } */ >> +/* { dg-require-effective-target tls } */ >> +/* { dg-options "-g -fPIC" } */ >> + >> +__thread int i; >> + >> +void >> +foo(void) >> +{ >> + i = 0; >> +} >> + >> +/* { dg-final { scan-assembler "data16\[ \t\]*leaq" } } */ >> +/* { dg-final { scan-assembler-not "\.byte\[ \t\]*0x66\n\[ \t\]*leaq" } } */
On 06/24/2018 11:59 PM, Jan Hubicka wrote: > Hi, > searching for other occurences I see: > jan@skylake:~/trunk/gcc/config/i386> grep ASM_BYTE *md *.c > i386.md: return ASM_BYTE "0x9e"; > i386.md: fputs (ASM_BYTE "0x66\n", asm_out_file); > i386.md: fputs (ASM_BYTE "0x66\n", asm_out_file); > i386.c: fputs (ASM_BYTE "0x48, 0x8d, 0xa4, 0x24, 0x00, 0x00, 0x00, 0x00\n", > i386.c: fputs (ASM_BYTE "0x8b, 0xff, 0x55, 0x8b, 0xec\n", asm_out_file); > i386.c: fputs ("\n" ASM_BYTE "0xf2\n\t", file); > i386.c: fputs ("\n" ASM_BYTE "0xf3\n\t", file); > i386.c: fprintf (file, "1:" ASM_BYTE "0x0f, 0x1f, 0x44, 0x00, 0x00\n"); > i386.c:#undef TARGET_ASM_BYTE_OP > i386.c:#define TARGET_ASM_BYTE_OP ASM_BYTE > I've just reviewed all these occurances, as well as the bigger-sized ASM_<size> ones, and indeed in a couple of places we get a breakpoint at an incorrect location, but AFAIU not in the middle of an insn as is the case here. > Perhaps we want to add new macro like ASM_INSN_BYTE which is used to output > such prefixes... Maybe. And I suspect that with the .insn/.endinsn construct I mentioned earlier in this thread, this new macro would be easier to implement. Thanks, - Tom
On 06/25/2018 08:25 AM, Tom de Vries wrote: > If we'd implemented something like this in gas: > ... > .insn > .byte 0x66 > .endinsn > ... > we could fix this more generically. Doesn't arm gas provide this functionality with some target-specific pseudo? It'd be good to copy that. ah, inst: @cindex @code{.inst} directive, ARM @item .inst @var{opcode} [ , @dots{} ] @itemx .inst.n @var{opcode} [ , @dots{} ] @itemx .inst.w @var{opcode} [ , @dots{} ] Generates the instruction corresponding to the numerical value @var{opcode}. @code{.inst.n} and @code{.inst.w} allow the Thumb instruction size to be specified explicitly, overriding the normal encoding rules. (ARM needs to distinguish data from insns to emit the mapping symbols needed for be8 endianness xforms). nathan
On 06/25/2018 02:45 PM, Nathan Sidwell wrote: > On 06/25/2018 08:25 AM, Tom de Vries wrote: > >> If we'd implemented something like this in gas: >> ... >> .insn >> .byte 0x66 >> .endinsn >> ... >> we could fix this more generically. > > Doesn't arm gas provide this functionality with some target-specific > pseudo? It'd be good to copy that. > > ah, inst: > > @cindex @code{.inst} directive, ARM > @item .inst @var{opcode} [ , @dots{} ] > @itemx .inst.n @var{opcode} [ , @dots{} ] > @itemx .inst.w @var{opcode} [ , @dots{} ] > Generates the instruction corresponding to the numerical value > @var{opcode}. > @code{.inst.n} and @code{.inst.w} allow the Thumb instruction size to be > specified explicitly, overriding the normal encoding rules. > > > (ARM needs to distinguish data from insns to emit the mapping symbols > needed for be8 endianness xforms). > Hmm, thanks for pointing that out. There's also something similar for s390 and riscv. For mips there's another form of .insn ( https://sourceware.org/binutils/docs/as/MIPS-insn.html#index-_002einsn ), which is similar to what I was proposing: ... 9.27.8 Directive to mark data as an instruction The .insn directive tells as that the following data is actually instructions. This makes a difference in MIPS 16 and microMIPS modes: when loading the address of a label which precedes instructions, as automatically adds 1 to the value, so that jumping to the loaded address will do the right thing. ... Thanks, - Tom
Hi Tom, > On 06/24/2018 11:56 PM, Jan Hubicka wrote: >>> Hi, >>> >>> [ The analysis of this PR was done at https://stackoverflow.com/a/33557963 , >>> November 2015. ] >>> >>> This tls sequence: >>> ... >>> 0x00 .byte 0x66 >>> 0x01 leaq x@tlsgd(%rip),%rdi >>> 0x08 .word 0x6666 >>> 0x0a rex64 >>> 0x0b call __tls_get_addr@plt >>> ... >>> starts with an insn prefix, produced using a .byte. >>> >>> When using a .loc before the sequence, it's associated with the next assembly >>> instruction, which is the leaq, so the .loc will end up pointing inside the >>> sequence rather than to the start of the sequence. And when the linker >>> relaxes the sequence, the .loc may end up pointing inside an insn. This will >>> cause an executable containing such a misplaced .loc to crash when gdb >>> continues from the associated breakpoint. >> >> Hmm, I am not sure why .byte should be non-instruction and .data should >> be instruction, >> but I assume gas simply behaves this way. >> > > Well, I suppose when encountering .byte/.value/.long/.quad, gas has no > way of knowing whether it's dealing with data or instructions, even in > the text section. An even if it would know it's dealing with > instructions, it wouldn't know where an instruction begins or ends. So > to me the behaviour seems reasonable. > > If we'd implemented something like this in gas: > ... > .insn > .byte 0x66 > .endinsn > ... > we could fix this more generically. > > Or maybe we'd want this, which allows us to express that the .byte is a > prefix to an existing insn: > ... > .insn > .byte 0x66 > leaq x@tlsgd(%rip),%rdi > .endinsn > ... > >> Don't we have also other cases wehre .byte is used to output instructions? >> >> Patch is OK (and probably should be backported after some soaking in mainline) >> > > Committed (after moving the testcase to gcc.target/i386). the new testcase FAILs on 32-bit Solaris/x86 FAIL: gcc.target/i386/pr86257.c scan-assembler data16[ \\t]*leaq and, according to gcc-testresults, also on i586-unknown-freebsd10.4 and x86_64-pc-linux-gnu. It's by design 64-bit only and should document that. Done as follows, tested on i386-pc-solaris2.11 (both multilibs), installed on mainline. Rainer
On Tue, Jun 26, 2018 at 11:20:32AM +0200, Rainer Orth wrote: > > Committed (after moving the testcase to gcc.target/i386). > > the new testcase FAILs on 32-bit Solaris/x86 > > FAIL: gcc.target/i386/pr86257.c scan-assembler data16[ \\t]*leaq > > and, according to gcc-testresults, also on i586-unknown-freebsd10.4 and > x86_64-pc-linux-gnu. Yeah, it failed for me with -m32 or -mx32, and also fails with -mtls-dialect=gnu2. So I've installed following on top of your patch as obvious. 2018-06-26 Jakub Jelinek <jakub@redhat.com> PR debug/86257 * gcc.target/i386/pr86257.c: Add -mtls-dialect=gnu to dg-options. --- gcc/testsuite/gcc.target/i386/pr86257.c.jj 2018-06-25 14:51:20.481986807 +0200 +++ gcc/testsuite/gcc.target/i386/pr86257.c 2018-06-26 12:32:10.284048124 +0200 @@ -1,7 +1,7 @@ /* { dg-require-effective-target lp64 } */ /* { dg-require-effective-target fpic } */ /* { dg-require-effective-target tls } */ -/* { dg-options "-g -fPIC" } */ +/* { dg-options "-g -fPIC -mtls-dialect=gnu" } */ __thread int i; Jakub
On 06/25/2018 03:02 PM, Tom de Vries wrote: > On 06/25/2018 02:45 PM, Nathan Sidwell wrote: >> On 06/25/2018 08:25 AM, Tom de Vries wrote: >> >>> If we'd implemented something like this in gas: >>> ... >>> .insn >>> .byte 0x66 >>> .endinsn >>> ... >>> we could fix this more generically. >> >> Doesn't arm gas provide this functionality with some target-specific >> pseudo? It'd be good to copy that. >> >> ah, inst: >> >> @cindex @code{.inst} directive, ARM >> @item .inst @var{opcode} [ , @dots{} ] >> @itemx .inst.n @var{opcode} [ , @dots{} ] >> @itemx .inst.w @var{opcode} [ , @dots{} ] >> Generates the instruction corresponding to the numerical value >> @var{opcode}. >> @code{.inst.n} and @code{.inst.w} allow the Thumb instruction size to be >> specified explicitly, overriding the normal encoding rules. >> >> >> (ARM needs to distinguish data from insns to emit the mapping symbols >> needed for be8 endianness xforms). >> > > Hmm, thanks for pointing that out. There's also something similar for > s390 and riscv. > > For mips there's another form of .insn ( > https://sourceware.org/binutils/docs/as/MIPS-insn.html#index-_002einsn > ), which is similar to what I was proposing: > ... > 9.27.8 Directive to mark data as an instruction > > The .insn directive tells as that the following data is actually > instructions. This makes a difference in MIPS 16 and microMIPS modes: > when loading the address of a label which precedes instructions, as > automatically adds 1 to the value, so that jumping to the loaded address > will do the right thing. > ... > Filed as PR23423 - "generic directive to mark data as insn" ( https://sourceware.org/bugzilla/show_bug.cgi?id=23423 ). Thanks, - Tom
diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md index eb77ef3c08f..6f2300057aa 100644 --- a/gcc/config/i386/i386.md +++ b/gcc/config/i386/i386.md @@ -14733,7 +14733,18 @@ "TARGET_64BIT" { if (!TARGET_X32) - fputs (ASM_BYTE "0x66\n", asm_out_file); + /* The .loc directive has effect for 'the immediately following assembly + instruction'. So for a sequence: + .loc f l + .byte x + insn1 + the 'immediately following assembly instruction' is insn1. + We want to emit an insn prefix here, but if we use .byte (as shown in + 'ELF Handling For Thread-Local Storage'), a preceding .loc will point + inside the insn sequence, rather than to the start. After relaxation + of the sequence by the linker, the .loc might point inside an insn. + Use data16 prefix instead, which doesn't have this problem. */ + fputs ("\tdata16", asm_out_file); output_asm_insn ("lea{q}\t{%E1@tlsgd(%%rip), %%rdi|rdi, %E1@tlsgd[rip]}", operands); if (TARGET_SUN_TLS || flag_plt || !HAVE_AS_IX86_TLS_GET_ADDR_GOT) diff --git a/gcc/testsuite/gcc.dg/pr86257.c b/gcc/testsuite/gcc.dg/pr86257.c new file mode 100644 index 00000000000..3287c190d36 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr86257.c @@ -0,0 +1,14 @@ +/* { dg-require-effective-target fpic } */ +/* { dg-require-effective-target tls } */ +/* { dg-options "-g -fPIC" } */ + +__thread int i; + +void +foo(void) +{ + i = 0; +} + +/* { dg-final { scan-assembler "data16\[ \t\]*leaq" } } */ +/* { dg-final { scan-assembler-not "\.byte\[ \t\]*0x66\n\[ \t\]*leaq" } } */