This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: AW: Software Quality Binutils
On Sat, Aug 18, 2018 at 08:31:30AM +0200, John Darrington wrote:
> On Thu, Aug 16, 2018 at 04:18:47PM +0000, Christoph Hazott wrote:
> Hi Jeff,
>
> yes of course! Do you have any suggestions?
>
> Regards,
>
> Christoph
>
> Just out of curiosity I quickly ran cppchack on the bfd directory (since
> that's rather central to a lot of things), filtered out a few uninteresting cases and
> got the result:
>
>
> [bfd/cpu-sh.c:468]: (error) Shifting 32-bit value by 32 bits is undefined behaviour
Not a bug. The MASK macro in opcodes/sh-opc.c protects against 32-bit
shifts.
#define MASK(LO,HI) ( LO < 1 ? ((1U << (HI + 1)) - 1) \
: HI > 30 ? (-1U << LO) \
: LO == HI ? (1U << LO) \
: (((1U << (HI + 1)) - 1) & (-1U << LO)))
Not very elegant, and it seems the author was wrongly afraid of 1 << 0
as well as 1 << 32. A slight variation of the last expression would
avoid 1 << 32:
#define MASK(LO,HI) (((1U << HI << 1) - 1) & (-1U << LO))
or even better
#define MASK(LO,HI) ((1U << HI << 1) - (1U << LO))
> [bfd/elf32-ppc.c:7944]: (error) Signed integer overflow for expression '2172846080+4'.
> [bfd/elf32-ppc.c:11404]: (error) Signed integer overflow for expression '2173435904+4'.
> [bfd/elf32-ppc.c:11431]: (error) Signed integer overflow for expression '2173435904+4'.
No bugs here either. The constants are hexadecimal, so if int is 32
bits, the expression should be of type "unsigned int". See 6.4.4.1
ISO C99 standard.
> [bfd/elf32-rl78.c:476]: (error) Division by zero.
> [bfd/elf32-rl78.c:532]: (error) Division by zero.
The divide by zero that this static analysis tool is reporting is
after an internal error. So that one doesn't matter too much.
However there is the possibility of a complex reloc expression that
actually contains a divide by zero, and the expression evaluation code
should be modified to report that error gracefully. Of course, static
analysis doesn't catch that divide by zero..
> [bfd/elf32-sh.c:6002]: (error) Shifting 32-bit value by 32 bits is undefined behaviour
> [bfd/elf32-sh.c:6014]: (error) Shifting 32-bit value by 32 bits is undefined behaviour
More uses of the MASK macro.
> [bfd/elf64-alpha.c:5041]: (error) Shifting a negative value is undefined behaviour
Well, yes, a right shift of a negative number is undefined according
to the C standard, but how many one's complement machines exist today?
(I believe the C standard is allowing for one's complement hardware,
or hardware without sign-copying right shift. Thus a compiler may
emit code that performs right shift as an unsigned shift regardless of
the type of the value being shifted.) elf64-alpha.c:5041 will work
fine with an unsigned right shift.
Note that there is a *lot* of code in binutils that assumes two's
complement arithmetic..
> [bfd/elf64-ppc.c:6942]: (error) Signed integer overflow for expression '4160815104+16'.
> [bfd/elf64-ppc.c:6958]: (error) Signed integer overflow for expression '3892379648+16'.
> [bfd/elf64-ppc.c:7013]: (error) Signed integer overflow for expression '4160815104+16'.
> [bfd/elf64-ppc.c:7029]: (error) Signed integer overflow for expression '3892379648+16'.
> [bfd/elf64-ppc.c:11319]: (error) Signed integer overflow for expression '3915579392+0'.
> [bfd/elf64-ppc.c:11320]: (error) Signed integer overflow for expression '3917676544+8'.
> [bfd/elf64-ppc.c:14216]: (error) Signed integer overflow for expression '4165009408+24'.
No bugs here either, except in the tool reporting a problem.
> [bfd/gen-aout.c:54]: (error) Resource leak: file
True, but totally unimportant in a generator program.
> [bfd/mach-o.c:5582]: (error) Signed integer overflow for expression '3221225472-67108864'.
Another expression that is unsigned.
> [bfd/mmo.c:497]: (error) Buffer is accessed out of bounds: valid_mmo_symbol_character_set
Nope. Another tool bug.
> [bfd/rs6000-core.c:311]: (error) Invalid number of character '(' when these macros are defined: 'AIX_5_CORE;AIX_CORE'.
> [bfd/rs6000-core.c:311]: (error) Invalid number of character '(' when these macros are defined: 'AIX_5_CORE;HAVE_ST_C_IMPL;AIX_CORE'.
> [bfd/rs6000-core.c:311]: (error) Invalid number of character '(' when these macros are defined: 'AIX_CORE'.
> [bfd/rs6000-core.c:311]: (error) Invalid number of character '(' when these macros are defined: 'AIX_CORE;AOUTHDR'.
> [bfd/rs6000-core.c:311]: (error) Invalid number of character '(' when these macros are defined: 'AIX_CORE;BFD64'.
> [bfd/rs6000-core.c:311]: (error) Invalid number of character '(' when these macros are defined: 'AIX_CORE;BFD64;CORE_VERSION_1'.
> [bfd/rs6000-core.c:311]: (error) Invalid number of character '(' when these macros are defined: 'AIX_CORE;CORE_VERSION_1'.
OK so this one is a real bug, easily noticed if anyone compiles this
code. Which demonstrates the old AIX code in binutils is dead.
>
> Some of those seem a little concerning at first glance ...
The concern should be for the usefulness/correctness of the tool
reporting these errors.
I'm going to fix rs6000-core.c and tidy opcodes/sh-opc.c and
bfd/mmo.c, but I won't accept patches to elf32-ppc.c and elf64-ppc.c
that needlessly add 'U' to constants.
--
Alan Modra
Australia Development Lab, IBM