[RFA] Undefined behaviour in include/coff/ti.h macros [was Re: BFD built failures with GCC 4.5]

[Dave Korn]

[ TIC4X/TIC54X maintainers CC'd. ]

On 23/04/2010 16:44, Dave Korn wrote:
> On 22/04/2010 18:58, Tom Tromey wrote:
>> I tried to build GDB (with --enable-targets=all) with GCC 4.5.
>>
>> I got the append errors.
>> I looked into them a bit, but I don't know anything about COFF or the
>> logic in ti.h (e.g. GET_SCNHDR_FLAGS).
> 
>   Thanks for spotting the problem; I'm on it now.

  Ouch, this one really turned out to open a whole bulk catering-size can of
worms.

  The underlying problem is that there are two different versions of the
section header struct in TI COFF format:

/* COFF0, COFF1 */
struct external_scnhdr_v01 {
	char		s_name[8];	/* section name			*/
	char		s_paddr[4];	/* physical address, aliased s_nlib */
	char		s_vaddr[4];	/* virtual address		*/
	char		s_size[4];	/* section size (in WORDS)      */
	char		s_scnptr[4];	/* file ptr to raw data for section */
	char		s_relptr[4];	/* file ptr to relocation	*/
	char		s_lnnoptr[4];	/* file ptr to line numbers	*/
	char		s_nreloc[2];	/* number of relocation entries	*/
	char		s_nlnno[2];	/* number of line number entries*/
	char		s_flags[2];	/* flags			*/
        char            s_reserved[1];  /* reserved                     */
        char            s_page[1];      /* section page number (LOAD)   */
};

/* COFF2 */
struct external_scnhdr {
	char		s_name[8];	/* section name			*/
	char		s_paddr[4];	/* physical address, aliased s_nlib */
	char		s_vaddr[4];	/* virtual address		*/
	char		s_size[4];	/* section size (in WORDS)      */
	char		s_scnptr[4];	/* file ptr to raw data for section */
	char		s_relptr[4];	/* file ptr to relocation	*/
	char		s_lnnoptr[4];	/* file ptr to line numbers	*/
	char		s_nreloc[4];	/* number of relocation entries	*/
	char		s_nlnno[4];	/* number of line number entries*/
	char		s_flags[4];	/* flags			*/
        char            s_reserved[2];  /* reserved                     */
        char            s_page[2];      /* section page number (LOAD)   */
};

  Notice how these are basically the same, except that the sizes of the last
few members, beginning at s_nreloc, are doubled in the COFF2 version.  The
tic4x/tic54x targets want to handle both these formats transparently, but
there's a problem: there's only one bfd_swap_section_header_in/out routine,
and it calls the various PUT/GET_SCNHDR_xxx macros with just a pointer to the
default struct external_scnhdr.

  In the current code, the solution is to apply a byte offset in the non-COFF2
case, which is hardcoded to the delta between the offsetof values of the
corresponding members in the two different versions of the structs, e.g.:

#define PUT_SCNHDR_FLAGS(ABFD, VAL, LOC) \
  (COFF2_P (ABFD) ? H_PUT_32 (ABFD, VAL, LOC) : H_PUT_16 (ABFD, VAL, (LOC) - 4))
#define GET_SCNHDR_PAGE(ABFD, LOC) \
  (COFF2_P (ABFD) ? H_GET_16 (ABFD, LOC) : (unsigned) H_GET_8 (ABFD, (LOC) - 7))

... and so on.  This is described by this comment:

/* COFF2 changes the offsets and sizes of these fields
   Assume we're dealing with the COFF2 scnhdr structure, and adjust
   accordingly.  Note: The GNU C versions of some of these macros
   are necessary in order to avoid compile time warnings triggered
   gcc's array bounds checking.  The PUT_SCNHDR_PAGE macro also has
   the advantage on not evaluating LOC twice.  */

and the "GNU C versions" to which it is referring replace the above
constructions with a statement-expression that attempts to avoid the array
bounds-checking error by dereferencing the pointer as a char*, e.g.:

#define PUT_SCNHDR_FLAGS(ABFD, VAL, LOC) \
  do \
    { \
      char * ptr = (LOC); \
      if (COFF2_P (ABFD)) \
	H_PUT_32 (ABFD, VAL, ptr); \
      else \
	H_PUT_16 (ABFD, VAL, ptr - 4); \
    } \
  while (0)
#define GET_SCNHDR_PAGE(ABFD, LOC) \
  ({ \
    unsigned page; \
    char * ptr = (LOC); \
    if (COFF2_P (ABFD)) \
      page = H_GET_16 (ABFD, ptr); \
    else \
      page = (unsigned) H_GET_8 (ABFD, ptr - 7); \
    page; \
  })

  Well, GCC just got cleverer, and now it can see right past that trick, and
it properly identifies that ptr is pointing to one of the char array members
of the struct, and complains about attempting to access the (e.g. again) -4th
or -7th element of an array of char[2] or char[1] (respectively).  That's a
real warning, because what we're doing there is pointer arithmetic, and
attempting to index a pointer right out of one array and into another: but
that's not allowed, as per the C standard; quoting here n1256, 6.5.6.8:

> If both the pointer operand and the result point to elements of the same
> array object, or one past the last element of the array object, the
> evaluation shall not produce an overflow; otherwise, the behavior is
> undefined. If the result points one past the last element of the array
> object, it shall not be used as the operand of a unary * operator that is
> evaluated.

  So, I guess that's undefined behaviour there.  Now, the optimal solution
would be to have an actual pointer to the actual type of the actual section
header, and dereference that to get directly at the actual member at whatever
its actual offsetof may be.  But that doesn't sit well with the structure of
coff_swap_scnhdr_in/out and the GET/SET_SCNHDR_XXXX macros that it uses,
because they take an lvalue of the dereferenced member of the section header
pointer, such as for example:

   scnhdr_int->s_flags = GET_SCNHDR_FLAGS (abfd, scnhdr_ext->s_flags);

and so there's no way from within the macro to change the type of the pointer
before it gets dereferenced.


  Ouch again.  So, the first draft at a solution I've come up with puts
wrappers around the various GET/SET_SCNHDR_ functions, that take the pointer
and the member as separate arguments:

   scnhdr_int->s_flags = GET_SCNHDR_FLAGS_WRAPPER (abfd, scnhdr_ext, s_flags);

where the default definition is

#ifndef GET_SCNHDR_FLAGS_WRAPPER
#define GET_SCNHDR_FLAGS_WRAPPER(ABFD, STRUCT, MEM) \
		GET_SCNHDR_FLAGS(ABFD, STRUCT->MEM)
#endif

which acts as the original code in all cases that don't define an override.
For TI COFF, I then add overrides that cast the pointer to the other kind of
section header before dereferencing it in the non-COFF2 case.  That, in turn,
would generate type-punning warnings without first unioning the two section
header structs, so I've ended up with a bunch of macros that look like the below:

union external_scnhdr_both {
	struct external_scnhdr_v01	v1;
	struct external_scnhdr		v2;
};

#define SCNHDR_BOTH union external_scnhdr_both

#define GET_SCNHDR_FLAGS_WRAPPER(ABFD, STRUCT, MEM) \
  ({ \
    int flags; \
    char *data = (char *)(STRUCT); \
    if (COFF2_P (ABFD)) \
      flags = H_GET_32 (ABFD, STRUCT->MEM); \
    else \
      flags = H_GET_16 (ABFD, ((SCNHDR_BOTH *)(data))->v1.MEM); \
    flags; \
  })


  Well, I think that's valid code; it compiles at all levels of
-Wstrict-aliasing without array bounds or type-punning warnings, but it's also
a bit ugly and fragile if any code ends up calling the GET/SET_SCNHDR_ macros
directly rather than the _WRAPPER version.

  I'm not sure what might make a better solution though.  Could we make the
different TI COFF formats use different bfd target vectors, without breaking
backward compatibility, or can anyone see a cleaner way to solve the problem
of accessing two different types of section header with only one set of code?

  Attached is a first draft of the fix (for TIC54X only, and untested as yet;
purely a PoC so far.)

    cheers,
      DaveK


-------------- next part --------------
A non-text attachment was scrubbed...
Name: tic-fix.diff
Type: text/x-c
Size: 11891 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20100424/8f936b70/attachment.bin>