diff mbox

tilegx-tdep: Correct aliasing errors in `tilegx_analyze_prologue'

Message ID alpine.DEB.2.00.1610150200220.31859@tp.orcam.me.uk
State Committed
Headers show

Commit Message

Maciej W. Rozycki Oct. 17, 2016, 3:18 p.m. UTC
Fix a load of aliasing build errors:

cc1plus: warnings being treated as errors
.../gdb/tilegx-tdep.c: In function 'CORE_ADDR tilegx_analyze_prologue(gdbarch*, CORE_ADDR, CORE_ADDR, tilegx_frame_cache*, frame_info*)':
.../gdb/tilegx-tdep.c:609: error: dereferencing pointer 'operands' does break strict-aliasing rules
.../gdb/tilegx-tdep.c:592: error: dereferencing pointer 'operands' does break strict-aliasing rules
.../gdb/tilegx-tdep.c:571: error: dereferencing pointer 'operands' does break strict-aliasing rules
[...]
.../gdb/tilegx-tdep.c:601: error: dereferencing pointer '<anonymous>' does break strict-aliasing rules
.../gdb/tilegx-tdep.c:601: note: initialized from here
cc1plus: error: dereferencing pointer 'operands' does break strict-aliasing rules
cc1plus: error: dereferencing pointer 'operands' does break strict-aliasing rules
.../gdb/tilegx-tdep.c:452: note: initialized from here
cc1plus: error: dereferencing pointer 'pretmp.896' does break strict-aliasing rules
cc1plus: note: initialized from here
cc1plus: error: dereferencing pointer 'pretmp.896' does break strict-aliasing rules
cc1plus: note: initialized from here
make[1]: *** [tilegx-tdep.o] Error 1

from an attempt to cast a `long long' pointer to an `int64_t' pointer, 
which may not necessarily be compatible types.  Use the `long long' type 
for the auxiliary variable then as this is the type of the structure 
member referred.

	gdb/
	* tilegx-tdep.c (tilegx_analyze_prologue): Use the `long long' 
	type for `operands'.
---
Hi,

 This was discovered in an `--enable-targets=all' `mips-mti-linux-gnu' 
build and may be dependent on the host type and compiler version, which 
are `x86_64-linux' and 4.4.7 here, respectively.  OK to apply?

  Maciej

gdb-tilegx-prologue-operands-fix.diff

Comments

Pedro Alves Oct. 17, 2016, 3:46 p.m. UTC | #1
On 10/17/2016 04:18 PM, Maciej W. Rozycki wrote:
> Fix a load of aliasing build errors:
> 
> cc1plus: warnings being treated as errors
> .../gdb/tilegx-tdep.c: In function 'CORE_ADDR tilegx_analyze_prologue(gdbarch*, CORE_ADDR, CORE_ADDR, tilegx_frame_cache*, frame_info*)':
> .../gdb/tilegx-tdep.c:609: error: dereferencing pointer 'operands' does break strict-aliasing rules
> .../gdb/tilegx-tdep.c:592: error: dereferencing pointer 'operands' does break strict-aliasing rules
> .../gdb/tilegx-tdep.c:571: error: dereferencing pointer 'operands' does break strict-aliasing rules
> [...]
> .../gdb/tilegx-tdep.c:601: error: dereferencing pointer '<anonymous>' does break strict-aliasing rules
> .../gdb/tilegx-tdep.c:601: note: initialized from here
> cc1plus: error: dereferencing pointer 'operands' does break strict-aliasing rules
> cc1plus: error: dereferencing pointer 'operands' does break strict-aliasing rules
> .../gdb/tilegx-tdep.c:452: note: initialized from here
> cc1plus: error: dereferencing pointer 'pretmp.896' does break strict-aliasing rules
> cc1plus: note: initialized from here
> cc1plus: error: dereferencing pointer 'pretmp.896' does break strict-aliasing rules
> cc1plus: note: initialized from here
> make[1]: *** [tilegx-tdep.o] Error 1
> 
> from an attempt to cast a `long long' pointer to an `int64_t' pointer, 
> which may not necessarily be compatible types.  Use the `long long' type 
> for the auxiliary variable then as this is the type of the structure 
> member referred.
> 
> 	gdb/
> 	* tilegx-tdep.c (tilegx_analyze_prologue): Use the `long long' 
> 	type for `operands'.
> ---
> Hi,
> 
>  This was discovered in an `--enable-targets=all' `mips-mti-linux-gnu' 
> build and may be dependent on the host type and compiler version, which 
> are `x86_64-linux' and 4.4.7 here, respectively.  OK to apply?
> 

Sure, please do ahead.

I'd be better if tilegx.h used uint32_t/uint64_t, etc,
but that can always be done separately by someone motivated.

Thanks,
Pedro Alves
Maciej W. Rozycki Oct. 18, 2016, 3:50 a.m. UTC | #2
On Mon, 17 Oct 2016, Pedro Alves wrote:

> > Fix a load of aliasing build errors:
> > 
> > cc1plus: warnings being treated as errors
> > .../gdb/tilegx-tdep.c: In function 'CORE_ADDR tilegx_analyze_prologue(gdbarch*, CORE_ADDR, CORE_ADDR, tilegx_frame_cache*, frame_info*)':
> > .../gdb/tilegx-tdep.c:609: error: dereferencing pointer 'operands' does break strict-aliasing rules
> > .../gdb/tilegx-tdep.c:592: error: dereferencing pointer 'operands' does break strict-aliasing rules
> > .../gdb/tilegx-tdep.c:571: error: dereferencing pointer 'operands' does break strict-aliasing rules
> > [...]
> > .../gdb/tilegx-tdep.c:601: error: dereferencing pointer '<anonymous>' does break strict-aliasing rules
> > .../gdb/tilegx-tdep.c:601: note: initialized from here
> > cc1plus: error: dereferencing pointer 'operands' does break strict-aliasing rules
> > cc1plus: error: dereferencing pointer 'operands' does break strict-aliasing rules
> > .../gdb/tilegx-tdep.c:452: note: initialized from here
> > cc1plus: error: dereferencing pointer 'pretmp.896' does break strict-aliasing rules
> > cc1plus: note: initialized from here
> > cc1plus: error: dereferencing pointer 'pretmp.896' does break strict-aliasing rules
> > cc1plus: note: initialized from here
> > make[1]: *** [tilegx-tdep.o] Error 1
> > 
> > from an attempt to cast a `long long' pointer to an `int64_t' pointer, 
> > which may not necessarily be compatible types.  Use the `long long' type 
> > for the auxiliary variable then as this is the type of the structure 
> > member referred.
> > 
> > 	gdb/
> > 	* tilegx-tdep.c (tilegx_analyze_prologue): Use the `long long' 
> > 	type for `operands'.
> > ---
> > Hi,
> > 
> >  This was discovered in an `--enable-targets=all' `mips-mti-linux-gnu' 
> > build and may be dependent on the host type and compiler version, which 
> > are `x86_64-linux' and 4.4.7 here, respectively.  OK to apply?
> > 
> 
> Sure, please do ahead.
> 
> I'd be better if tilegx.h used uint32_t/uint64_t, etc,
> but that can always be done separately by someone motivated.

 The issue there is `tilegx.h' is shared with binutils which have not yet 
moved past C89 I believe.  Of course the use of `long long' is already 
non-C89, however it's been there with some C89 compilers before `stdint.h' 
types have been introduced.  We could use `bfd_uint64_t' instead, but that 
would make `tilegx.h' depend on a BFD header.  So it's not an obvious call 
and therefore I agree it's best left to someone who has an actual interest 
with the target.

 Committed now, thanks for your review.

  Maciej
Pedro Alves Oct. 18, 2016, 9:22 a.m. UTC | #3
On 10/18/2016 04:50 AM, Maciej W. Rozycki wrote:
>> > 
>> > Sure, please do ahead.
>> > 
>> > I'd be better if tilegx.h used uint32_t/uint64_t, etc,
>> > but that can always be done separately by someone motivated.

>  The issue there is `tilegx.h' is shared with binutils which have not yet 
> moved past C89 I believe.  Of course the use of `long long' is already 
> non-C89, however it's been there with some C89 compilers before `stdint.h' 
> types have been introduced.  We could use `bfd_uint64_t' instead, but that 
> would make `tilegx.h' depend on a BFD header.  So it's not an obvious call 
> and therefore I agree it's best left to someone who has an actual interest 
> with the target.

Yeah, C89 on paper.  :-)  It's certainly fine to use unadorned uint64_t etc.
directly nowadays.  Even if the host compiler is so old that it doesn't
support those, we have "bfd_stdint.h" to fill the gap:

 opcodes/aarch64-dis.h:23:#include "bfd_stdint.h"
 opcodes/nds32-dis.c:30:#include "bfd_stdint.h"
 opcodes/aarch64-dis.c:22:#include "bfd_stdint.h"
 ld/elf-hints-local.h:28:#include "bfd_stdint.h"

Something like 'grep "u\?int\(8\|16\|32\|64\)_t" -rn'
will show many uses of uint32_t, etc. (not bfd_uint*_t) throughout
opcodes, bfd, ld, etc.  I won't be surprised at all if someone
missed adding bfd_stdint.h for some of them and they've been
working in practice for a long while.

(better than bfd_stdint.h would be to use gnulib for the whole
toolchain, not just for gdb, but that's yet another matter.)

As I've been suggesting in recent C++11 discussions,
IMO, what we claim to support in theory matters significantly
less than what we actually support in practice.  And in
practice, all it matters is which compilers (and their versions),
not standards, people care about actually using and supporting.
Standards give us guidelines, not final answers.

>  Committed now, thanks for your review.

Thanks.

Thanks,
Pedro Alves
diff mbox

Patch

Index: binutils/gdb/tilegx-tdep.c
===================================================================
--- binutils.orig/gdb/tilegx-tdep.c	2016-10-05 00:58:09.000000000 +0100
+++ binutils/gdb/tilegx-tdep.c	2016-10-15 01:29:35.689998335 +0100
@@ -449,7 +449,7 @@  tilegx_analyze_prologue (struct gdbarch*
       for (i = 0; i < num_insns; i++)
 	{
 	  struct tilegx_decoded_instruction *this_insn = &decoded[i];
-	  int64_t *operands = (int64_t *) this_insn->operand_values;
+	  long long *operands = this_insn->operand_values;
 	  const struct tilegx_opcode *opcode = this_insn->opcode;
 
 	  switch (opcode->mnemonic)