[PATCH] gdb: x86: fix x32 builds with inline asm

[PATCH] gdb: x86: fix x32 builds with inline asm

[Mike Frysinger]

The inline assembly fails with x32 targets due to pushq used with a 32bit
register.  Since the assembler can do the right thing with a "push" insn
and a 32bit or 64bit register (i.e. it'll pushl or pushq as needed), just
use that insn.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>

2012-01-08  Mike Frysinger  <vapier@gentoo.org>

	* common/linux-ptrace.c (linux_ptrace_test_ret_to_nx): Change pushl
	to push in i386 inline asm and %esp to sp.  Delete __i386__ check
	and __x86_64__ inline asm.
---
Note: I'm not 100% on the "sp" constraint.  Seems to work, and reading
	the gcc constraints list looks like it aliases sp/esp/rsp to the same
	thing.

 gdb/common/linux-ptrace.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/gdb/common/linux-ptrace.c b/gdb/common/linux-ptrace.c
index 761ef59..abb502c 100644
--- a/gdb/common/linux-ptrace.c
+++ b/gdb/common/linux-ptrace.c
@@ -103,21 +103,11 @@ linux_ptrace_test_ret_to_nx (void)
 		 strerror (errno));
       else
 	{
-#if defined __i386__
-	  asm volatile ("pushl %0;"
-			".globl linux_ptrace_test_ret_to_nx_instr;"
-			"linux_ptrace_test_ret_to_nx_instr:"
-			"ret"
-			: : "r" (return_address) : "%esp", "memory");
-#elif defined __x86_64__
-	  asm volatile ("pushq %0;"
+	  asm volatile ("push %0;"
 			".globl linux_ptrace_test_ret_to_nx_instr;"
 			"linux_ptrace_test_ret_to_nx_instr:"
 			"ret"
-			: : "r" (return_address) : "%rsp", "memory");
-#else
-# error "!__i386__ && !__x86_64__"
-#endif
+			: : "r" (return_address) : "sp", "memory");
 	  gdb_assert_not_reached ("asm block did not terminate");
 	}
 
-- 
1.8.0.2

Re: [PATCH] gdb: x86: fix x32 builds with inline asm

[H.J. Lu]

On Tue, Jan 8, 2013 at 7:01 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> The inline assembly fails with x32 targets due to pushq used with a 32bit
> register.  Since the assembler can do the right thing with a "push" insn
> and a 32bit or 64bit register (i.e. it'll pushl or pushq as needed), just
> use that insn.
>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>
> 2012-01-08  Mike Frysinger  <vapier@gentoo.org>
>
>         * common/linux-ptrace.c (linux_ptrace_test_ret_to_nx): Change pushl
>         to push in i386 inline asm and %esp to sp.  Delete __i386__ check
>         and __x86_64__ inline asm.
> ---
> Note: I'm not 100% on the "sp" constraint.  Seems to work, and reading
>         the gcc constraints list looks like it aliases sp/esp/rsp to the same
>         thing.
>
>  gdb/common/linux-ptrace.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/gdb/common/linux-ptrace.c b/gdb/common/linux-ptrace.c
> index 761ef59..abb502c 100644
> --- a/gdb/common/linux-ptrace.c
> +++ b/gdb/common/linux-ptrace.c
> @@ -103,21 +103,11 @@ linux_ptrace_test_ret_to_nx (void)
>                  strerror (errno));
>        else
>         {
> -#if defined __i386__
> -         asm volatile ("pushl %0;"
> -                       ".globl linux_ptrace_test_ret_to_nx_instr;"
> -                       "linux_ptrace_test_ret_to_nx_instr:"
> -                       "ret"
> -                       : : "r" (return_address) : "%esp", "memory");
> -#elif defined __x86_64__
> -         asm volatile ("pushq %0;"
> +         asm volatile ("push %0;"
>                         ".globl linux_ptrace_test_ret_to_nx_instr;"
>                         "linux_ptrace_test_ret_to_nx_instr:"
>                         "ret"
> -                       : : "r" (return_address) : "%rsp", "memory");
> -#else
> -# error "!__i386__ && !__x86_64__"
> -#endif
> +                       : : "r" (return_address) : "sp", "memory");
>           gdb_assert_not_reached ("asm block did not terminate");
>         }
>

I think we should keep

#else
# error "!__i386__ && !__x86_64__"
#endif

Thanks.

-- 
H.J.

Re: [PATCH] gdb: x86: fix x32 builds with inline asm

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1610 bytes --]

On Tuesday 08 January 2013 10:36:11 H.J. Lu wrote:
> On Tue, Jan 8, 2013 at 7:01 AM, Mike Frysinger wrote:
> > --- a/gdb/common/linux-ptrace.c
> > +++ b/gdb/common/linux-ptrace.c
> > @@ -103,21 +103,11 @@ linux_ptrace_test_ret_to_nx (void)
> >                  strerror (errno));
> >        else
> >         {
> > -#if defined __i386__
> > -         asm volatile ("pushl %0;"
> > -                       ".globl linux_ptrace_test_ret_to_nx_instr;"
> > -                       "linux_ptrace_test_ret_to_nx_instr:"
> > -                       "ret"
> > -                       : : "r" (return_address) : "%esp", "memory");
> > -#elif defined __x86_64__
> > -         asm volatile ("pushq %0;"
> > +         asm volatile ("push %0;"
> >                         ".globl linux_ptrace_test_ret_to_nx_instr;"
> >                         "linux_ptrace_test_ret_to_nx_instr:"
> >                         "ret"
> > -                       : : "r" (return_address) : "%rsp", "memory");
> > -#else
> > -# error "!__i386__ && !__x86_64__"
> > -#endif
> > +                       : : "r" (return_address) : "sp", "memory");
> >           gdb_assert_not_reached ("asm block did not terminate");
> >         }
> 
> I think we should keep
> 
> #else
> # error "!__i386__ && !__x86_64__"
> #endif

that #else case is currently impossible to hit because the whole func is 
protected by:
#if defined __i386__ || defined __x86_64__

i'm guessing the #else was there only to make sure that some asm test was 
performed, but since there is no conditional asm, that's no longer an issue
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] gdb: x86: fix x32 builds with inline asm

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1232 bytes --]

On Tuesday 08 January 2013 10:01:20 Mike Frysinger wrote:
> -#if defined __i386__
> -	  asm volatile ("pushl %0;"
> -			".globl linux_ptrace_test_ret_to_nx_instr;"
> -			"linux_ptrace_test_ret_to_nx_instr:"
> -			"ret"
> -			: : "r" (return_address) : "%esp", "memory");
> -#elif defined __x86_64__
> -	  asm volatile ("pushq %0;"
> +	  asm volatile ("push %0;"
>  			".globl linux_ptrace_test_ret_to_nx_instr;"
>  			"linux_ptrace_test_ret_to_nx_instr:"
>  			"ret"
> -			: : "r" (return_address) : "%rsp", "memory");
> -#else
> -# error "!__i386__ && !__x86_64__"
> -#endif
> +			: : "r" (return_address) : "sp", "memory");

hrm, this works for -m32 and -m64, but doesn't actually help with -mx32.  this 
doesn't seem to line up with my expectations.  can you suggest something here 
H.J. Lu ?

$ cat test.c
main() { asm volatile ("push %0; ret;" : : "r"(main) : "sp", "memory"); }

$ gcc -m32 test.c
   8:   50                      push   %eax
   9:   c3                      ret    

$ gcc -m64 test.c
   9:   50                      push   %rax
   a:   c3                      retq   

$ gcc -mx32 test.c
test.c: Assembler messages:
test.c:2: Error: operand type mismatch for `push'
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] gdb: x86: fix x32 builds with inline asm

[H.J. Lu]

On Tue, Jan 8, 2013 at 7:59 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Tuesday 08 January 2013 10:01:20 Mike Frysinger wrote:
>> -#if defined __i386__
>> -       asm volatile ("pushl %0;"
>> -                     ".globl linux_ptrace_test_ret_to_nx_instr;"
>> -                     "linux_ptrace_test_ret_to_nx_instr:"
>> -                     "ret"
>> -                     : : "r" (return_address) : "%esp", "memory");
>> -#elif defined __x86_64__
>> -       asm volatile ("pushq %0;"
>> +       asm volatile ("push %0;"
>>                       ".globl linux_ptrace_test_ret_to_nx_instr;"
>>                       "linux_ptrace_test_ret_to_nx_instr:"
>>                       "ret"
>> -                     : : "r" (return_address) : "%rsp", "memory");
>> -#else
>> -# error "!__i386__ && !__x86_64__"
>> -#endif
>> +                     : : "r" (return_address) : "sp", "memory");
>
> hrm, this works for -m32 and -m64, but doesn't actually help with -mx32.  this
> doesn't seem to line up with my expectations.  can you suggest something here
> H.J. Lu ?
>
> $ cat test.c
> main() { asm volatile ("push %0; ret;" : : "r"(main) : "sp", "memory"); }
>
> $ gcc -m32 test.c
>    8:   50                      push   %eax
>    9:   c3                      ret
>
> $ gcc -m64 test.c
>    9:   50                      push   %rax
>    a:   c3                      retq
>
> $ gcc -mx32 test.c
> test.c: Assembler messages:
> test.c:2: Error: operand type mismatch for `push'
> -mike

Can you try this?


-- 
H.J.
---
diff --git a/gdb/common/linux-ptrace.c b/gdb/common/linux-ptrace.c
index 761ef59..cc08b6a 100644
--- a/gdb/common/linux-ptrace.c
+++ b/gdb/common/linux-ptrace.c
@@ -103,21 +103,16 @@ linux_ptrace_test_ret_to_nx (void)
 		 strerror (errno));
       else
 	{
-#if defined __i386__
-	  asm volatile ("pushl %0;"
-			".globl linux_ptrace_test_ret_to_nx_instr;"
-			"linux_ptrace_test_ret_to_nx_instr:"
-			"ret"
-			: : "r" (return_address) : "%esp", "memory");
-#elif defined __x86_64__
-	  asm volatile ("pushq %0;"
+	  asm volatile ("push %0;"
 			".globl linux_ptrace_test_ret_to_nx_instr;"
 			"linux_ptrace_test_ret_to_nx_instr:"
 			"ret"
-			: : "r" (return_address) : "%rsp", "memory");
+#ifdef __x86_64__
+			: : "r" ((uint64_t) (uintptr_t) (return_address))
 #else
-# error "!__i386__ && !__x86_64__"
+			: : "r" (return_address)
 #endif
+			: "sp", "memory");
 	  gdb_assert_not_reached ("asm block did not terminate");
 	}

Re: [PATCH] gdb: x86: fix x32 builds with inline asm

[Jan Kratochvil]

On Tue, 08 Jan 2013 16:52:34 +0100, Mike Frysinger wrote:
> On Tuesday 08 January 2013 10:36:11 H.J. Lu wrote:
> > I think we should keep
> > 
> > #else
> > # error "!__i386__ && !__x86_64__"
> > #endif
> 
> that #else case is currently impossible to hit because the whole func is 
> protected by:
> #if defined __i386__ || defined __x86_64__
> 
> i'm guessing the #else was there only to make sure that some asm test was 
> performed, but since there is no conditional asm, that's no longer an issue

I agree and I do not mind either way.


Jan

Re: [PATCH] gdb: x86: fix x32 builds with inline asm

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1515 bytes --]

On Tuesday 08 January 2013 11:54:32 H.J. Lu wrote:
> On Tue, Jan 8, 2013 at 7:59 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> > On Tuesday 08 January 2013 10:01:20 Mike Frysinger wrote:
> >> +       asm volatile ("push %0;"
> >>                       ".globl linux_ptrace_test_ret_to_nx_instr;"
> >>                       "linux_ptrace_test_ret_to_nx_instr:"
> >>                       "ret"
> >> -                     : : "r" (return_address) : "%rsp", "memory");
> >> +                     : : "r" (return_address) : "sp", "memory");
> > 
> > hrm, this works for -m32 and -m64, but doesn't actually help with -mx32. 
> > this doesn't seem to line up with my expectations.  can you suggest
> > something here H.J. Lu ?
> > 
> > $ cat test.c
> > main() { asm volatile ("push %0; ret;" : : "r"(main) : "sp", "memory"); }
> > 
> > $ gcc -m32 test.c
> >    8:   50                      push   %eax
> >    9:   c3                      ret
> > $ gcc -m64 test.c
> >    9:   50                      push   %rax
> >    a:   c3                      retq
> > 
> > $ gcc -mx32 test.c
> > test.c: Assembler messages:
> > test.c:2: Error: operand type mismatch for `push'
> 
> Can you try this?
>
> +#ifdef __x86_64__
> +                       : : "r" ((uint64_t) (uintptr_t) (return_address))

that seems to work for me, but seems wrong.  i guess while most 32/64 bit 
issues can be handled transparently, the stack still only allows 
pushing/popping of 64bit values in x32 mode.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] gdb: x86: fix x32 builds with inline asm

[H.J. Lu]

On Tue, Jan 8, 2013 at 9:45 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Tuesday 08 January 2013 11:54:32 H.J. Lu wrote:
>> On Tue, Jan 8, 2013 at 7:59 AM, Mike Frysinger <vapier@gentoo.org> wrote:
>> > On Tuesday 08 January 2013 10:01:20 Mike Frysinger wrote:
>> >> +       asm volatile ("push %0;"
>> >>                       ".globl linux_ptrace_test_ret_to_nx_instr;"
>> >>                       "linux_ptrace_test_ret_to_nx_instr:"
>> >>                       "ret"
>> >> -                     : : "r" (return_address) : "%rsp", "memory");
>> >> +                     : : "r" (return_address) : "sp", "memory");
>> >
>> > hrm, this works for -m32 and -m64, but doesn't actually help with -mx32.
>> > this doesn't seem to line up with my expectations.  can you suggest
>> > something here H.J. Lu ?
>> >
>> > $ cat test.c
>> > main() { asm volatile ("push %0; ret;" : : "r"(main) : "sp", "memory"); }
>> >
>> > $ gcc -m32 test.c
>> >    8:   50                      push   %eax
>> >    9:   c3                      ret
>> > $ gcc -m64 test.c
>> >    9:   50                      push   %rax
>> >    a:   c3                      retq
>> >
>> > $ gcc -mx32 test.c
>> > test.c: Assembler messages:
>> > test.c:2: Error: operand type mismatch for `push'
>>
>> Can you try this?
>>
>> +#ifdef __x86_64__
>> +                       : : "r" ((uint64_t) (uintptr_t) (return_address))
>
> that seems to work for me, but seems wrong.  i guess while most 32/64 bit
> issues can be handled transparently, the stack still only allows
> pushing/popping of 64bit values in x32 mode.
> -mike

I believe it is correct for x32 since "ret" will pop 64bit as return address
for x32.

-- 
H.J.

[PATCH v2] gdb: x86: fix x32 builds with inline asm

[Mike Frysinger]

We need to cast the pointer up to 64bits so that the push works on x32
targets.  For 64bit targets, this makes no difference.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>

2012-01-08  Mike Frysinger  <vapier@gentoo.org>

	* common/linux-ptrace.c (linux_ptrace_test_ret_to_nx) [__x86_64__]:
	Cast return_address to 64bits.
---
v2
	- just cast the value in the x86_64 path to fix x32 builds

 gdb/common/linux-ptrace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/common/linux-ptrace.c b/gdb/common/linux-ptrace.c
index 761ef59..886be80 100644
--- a/gdb/common/linux-ptrace.c
+++ b/gdb/common/linux-ptrace.c
@@ -114,7 +114,8 @@ linux_ptrace_test_ret_to_nx (void)
 			".globl linux_ptrace_test_ret_to_nx_instr;"
 			"linux_ptrace_test_ret_to_nx_instr:"
 			"ret"
-			: : "r" (return_address) : "%rsp", "memory");
+			: : "r" ((uint64_t) (uintptr_t) return_address)
+			: "%rsp", "memory");
 #else
 # error "!__i386__ && !__x86_64__"
 #endif
-- 
1.8.0.2

Re: [PATCH v2] gdb: x86: fix x32 builds with inline asm

[Jan Kratochvil]

On Tue, 08 Jan 2013 19:44:59 +0100, Mike Frysinger wrote:
> 	* common/linux-ptrace.c (linux_ptrace_test_ret_to_nx) [__x86_64__]:
> 	Cast return_address to 64bits.

OK for commit.


Thanks,
Jan

Re: [PATCH v2] gdb: x86: fix x32 builds with inline asm

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 324 bytes --]

On Tuesday 08 January 2013 13:52:29 Jan Kratochvil wrote:
> On Tue, 08 Jan 2013 19:44:59 +0100, Mike Frysinger wrote:
> > 	* common/linux-ptrace.c (linux_ptrace_test_ret_to_nx) [__x86_64__]:
> > 	Cast return_address to 64bits.
> 
> OK for commit.

mmm i forgot to mention this is needed on the 7.5 branch too
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] gdb: x86: fix x32 builds with inline asm

[Jan Kratochvil]

On Tue, 08 Jan 2013 20:21:15 +0100, Mike Frysinger wrote:
> mmm i forgot to mention this is needed on the 7.5 branch too

7.5.1 has been already released and there won't be 7.5.2, rather 7.6 is going
to be released:
	http://sourceware.org/gdb/wiki/GDB_7.6_Release


Jan