diff mbox series

[PR86257,i386/debug] Fix insn prefix in tls_global_dynamic_64_<mode>

Message ID 20180624115027.zjjyorpdpb32ekm6@localhost.localdomain
State New
Headers show
Series [PR86257,i386/debug] Fix insn prefix in tls_global_dynamic_64_<mode> | expand

Commit Message

Tom de Vries June 24, 2018, 11:50 a.m. UTC 
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.

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(-)

Comments

Jan Hubicka June 24, 2018, 9:56 p.m. UTC | #1 
> 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" } } */
Jan Hubicka June 24, 2018, 9:59 p.m. UTC | #2 
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
Tom de Vries June 25, 2018, 12:25 p.m. UTC | #3 
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" } } */
Tom de Vries June 25, 2018, 12:32 p.m. UTC | #4 
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
Nathan Sidwell June 25, 2018, 12:45 p.m. UTC | #5 
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
Tom de Vries June 25, 2018, 1:02 p.m. UTC | #6 
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
Rainer Orth June 26, 2018, 9:20 a.m. UTC | #7 
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
Jakub Jelinek June 26, 2018, 10:37 a.m. UTC | #8 
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
Tom de Vries July 17, 2018, 4:19 p.m. UTC | #9 
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 mbox series

Patch

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" } } */