[PATCH] 32 bit-ism in lm32-tdep.c (and some sloppy macros)

[PATCH] 32 bit-ism in lm32-tdep.c (and some sloppy macros)

[Werner Almesberger]

Hello,

since at least gdb 7.1, stack traces (where or bt) of LM32 failed
on 64 bit hosts after just a few frames with

  Backtrace stopped: previous frame inner to this frame (corrupt stack?)

The reason for this is that stack frames weren't unwound correctly.
The bug is in the LM32_IMM16 macro, which tries to do sign expansion
by shifting the value to the left edge of a long, expecting it to
be 32 bits. This trick produces incorrect results on 64 bit systems.

I've observed this problem in all gdb versions I tried, namely 7.1
through 7.3.1.

The patch below delegates the work of figuring out such details to
the compiler. I've also taken the liberty of protecting all the
macro arguments, just in case.

- Werner

--- gdb-7.3.1/gdb/lm32-tdep.c.orig	2011-11-11 18:03:54.000000000 -0300
+++ gdb-7.3.1/gdb/lm32-tdep.c	2011-11-11 18:04:24.000000000 -0300
@@ -40,11 +40,11 @@
 #include "gdb_string.h"
 
 /* Macros to extract fields from an instruction.  */
-#define LM32_OPCODE(insn)       ((insn >> 26) & 0x3f)
-#define LM32_REG0(insn)         ((insn >> 21) & 0x1f)
-#define LM32_REG1(insn)         ((insn >> 16) & 0x1f)
-#define LM32_REG2(insn)         ((insn >> 11) & 0x1f)
-#define LM32_IMM16(insn)        ((((long)insn & 0xffff) << 16) >> 16)
+#define LM32_OPCODE(insn)       (((insn) >> 26) & 0x3f)
+#define LM32_REG0(insn)         (((insn) >> 21) & 0x1f)
+#define LM32_REG1(insn)         (((insn) >> 16) & 0x1f)
+#define LM32_REG2(insn)         (((insn) >> 11) & 0x1f)
+#define LM32_IMM16(insn)        ((long) (int16_t) (insn))
 
 struct gdbarch_tdep
 {
_______________________________________________
http://lists.milkymist.org/listinfo.cgi/devel-milkymist.org
IRC: #milkymist@Freenode

----- End forwarded message -----

Re: [PATCH] 32 bit-ism in lm32-tdep.c (and some sloppy macros)

[Tom Tromey]

>>>>> "Werner" == Werner Almesberger <werner@almesberger.net> writes:

Werner> The reason for this is that stack frames weren't unwound correctly.
Werner> The bug is in the LM32_IMM16 macro, which tries to do sign expansion
Werner> by shifting the value to the left edge of a long, expecting it to
Werner> be 32 bits. This trick produces incorrect results on 64 bit systems.

Thanks for the patch.

It needs a ChangeLog entry.

Werner> The patch below delegates the work of figuring out such details to
Werner> the compiler. I've also taken the liberty of protecting all the
Werner> macro arguments, just in case.

Werner> -#define LM32_IMM16(insn)        ((((long)insn & 0xffff) << 16) >> 16)
[...]
Werner> +#define LM32_IMM16(insn)        ((long) (int16_t) (insn))

I was a little surprised to find out we already use int16_t in gdb.
Anyway, it seems that the macro would be more obvious as:

#define LM32_IMM16(insn) ((long) ((insn) & 0xffff))

WDYT?

Tom

RE: [PATCH] 32 bit-ism in lm32-tdep.c (and some sloppy macros)

[Jon Beniston]

Hi Tom,

 
> Werner> -#define LM32_IMM16(insn)        ((((long)insn & 0xffff) << 16) >>
> 16)
> [...]
> Werner> +#define LM32_IMM16(insn)        ((long) (int16_t) (insn))
> 
> I was a little surprised to find out we already use int16_t in gdb.
> Anyway, it seems that the macro would be more obvious as:
> 
> #define LM32_IMM16(insn) ((long) ((insn) & 0xffff))
> 
> WDYT?

I'm not sure that sign-extends?

Jon

Re: [PATCH] 32 bit-ism in lm32-tdep.c (and some sloppy macros)

[Tom Tromey]

>>>>> "Jon" == Jon Beniston <jon@beniston.com> writes:

Jon> I'm not sure that sign-extends?

Yeah.  What am I thinking?

The original patch is OK with a ChangeLog entry.

Tom

Re: [PATCH] 32 bit-ism in lm32-tdep.c (and some sloppy macros)

[Pedro Alves]

On Monday 14 November 2011 15:48:48, Tom Tromey wrote:

> I was a little surprised to find out we already use int16_t in gdb.

We pull stdint.h from gnulib.

-- 
Pedro Alves

Re: [PATCH] 32 bit-ism in lm32-tdep.c (and some sloppy macros)

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> On Monday 14 November 2011 15:48:48, Tom Tromey wrote:
>> I was a little surprised to find out we already use int16_t in gdb.

Pedro> We pull stdint.h from gnulib.

I was meaning to ask ... do we have a gnulib update policy?
I wanted to pull in stdbool.h and start using bool in gdb; plus maybe
some other bits so we can use O_CLOEXEC and friends (but only maybe --
it isn't clear to me that gnulib is the best way to tackle this
problem).  Anyway, then I noticed that the existing files are not
up-to-date against gnulib git.

It seemed to me that updating now, just before a release, was maybe not
the best time.  Any thoughts?

Tom

Re: [PATCH] 32 bit-ism in lm32-tdep.c (and some sloppy macros)

[Pedro Alves]

On Monday 14 November 2011 16:20:36, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro> On Monday 14 November 2011 15:48:48, Tom Tromey wrote:
> >> I was a little surprised to find out we already use int16_t in gdb.
> 
> Pedro> We pull stdint.h from gnulib.
> 
> I was meaning to ask ... do we have a gnulib update policy?

I don't think we do.  I think we've updated about twice only since
adding gnulib, and it was because there was something new in gnulib
that we needed.  I've done it once.

> I wanted to pull in stdbool.h and start using bool in gdb; plus maybe
> some other bits so we can use O_CLOEXEC and friends (but only maybe --
> it isn't clear to me that gnulib is the best way to tackle this
> problem).  Anyway, then I noticed that the existing files are not
> up-to-date against gnulib git.
> 
> It seemed to me that updating now, just before a release, was maybe not
> the best time.  Any thoughts?

Agreed.  After release sounds best.  Do you know if gnulib follows any sort
of stable release/period in their trunk?  That is, is there any time that
is better for pulling current gnulib state that is better or worse
then others, in terms of pulling in gnulib bugs or works-in-progress?

-- 
Pedro Alves

Re: [PATCH] 32 bit-ism in lm32-tdep.c (and some sloppy macros)

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> Agreed.  After release sounds best.  Do you know if gnulib
Pedro> follows any sort of stable release/period in their trunk?  That
Pedro> is, is there any time that is better for pulling current gnulib
Pedro> state that is better or worse then others, in terms of pulling in
Pedro> gnulib bugs or works-in-progress?

I asked, and gnulib doesn't really do this.

Tom

Re: [PATCH] 32 bit-ism in lm32-tdep.c (and some sloppy macros)

[Mark Kettenis]

> From: Tom Tromey <tromey@redhat.com>
> Date: Mon, 14 Nov 2011 09:20:36 -0700
> 
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro> On Monday 14 November 2011 15:48:48, Tom Tromey wrote:
> >> I was a little surprised to find out we already use int16_t in gdb.
> 
> Pedro> We pull stdint.h from gnulib.
> 
> I was meaning to ask ... do we have a gnulib update policy?
> I wanted to pull in stdbool.h and start using bool in gdb; plus maybe
> some other bits so we can use O_CLOEXEC and friends (but only maybe --
> it isn't clear to me that gnulib is the best way to tackle this
> problem).  Anyway, then I noticed that the existing files are not
> up-to-date against gnulib git.
> 
> It seemed to me that updating now, just before a release, was maybe not
> the best time.  Any thoughts?

I'd say we should only update if there is a specific need.

Re: [PATCH] 32 bit-ism in lm32-tdep.c (and some sloppy macros)

[Mike Frysinger]

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

On Monday 14 November 2011 11:20:36 Tom Tromey wrote:
> I wanted to pull in stdbool.h and start using bool in gdb

+1
-mike

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