Software Quality Binutils

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

Software Quality Binutils

Christoph Hazott
Hi,


first of all I quickly want to introduce myself. My name is Christoph Hazott, I life in Austria but am originally from Germany. Currently I'm employed at Infineon as Test Development Engineer for RADAR Products and made my MSc. Degree in Embedded Systems Design at the University of Applied Sciences Upper Austria in 2012.

I'm very fond of Free Software but have to say that due to (in my opinion) software entropy with such a high amount of contributors and functionality the tools have become smelly.


I sat down now for one day and made a static code analysis of the binutils (because they are the major part of the toolchain). I uploaded the code of version 2.30.90 (last snapshot) to my public GitHub (https://github.com/h4z4rt/binutils) and connected it to sonar. In a virtual machine I executed the scanner with a Linux From Scratch configuration and the results where  uploaded to https://sonarcloud.io/organizations/h4z4rt-github/projects  and can be viewed there.



The results show 535 Bugs, 804 Vulnerabilities, 8.5k Code Smells and > 11.7% duplications.



Because of this I would like to contribute to the binutils project by setting up an infrastructure for static code analysis (and further...).

I would be happy if you would liketo have me to contribute to the binutils like this.



Looking forward to your supply.



Regards,



Christoph



PS: I'm not fixed on any code analysis tools I just used sonar because I personally made the best experience with it.











Reply | Threaded
Open this post in threaded view
|

Re: Software Quality Binutils

John Darrington-4
Hi Christoph,

I'm sure that if you identify any particular issues and send
patches, then they'll be carefully considered by the relevant person.

However I'm equally sure that if you send patches simply because they
shut sonar up then most will be rejected.   Sonar in its default
cofiguration is highly promiscuous - it includes criteria from
committees such as MISRA who in my opionion have failed to grasp many
principles of coding, and their rules achive the opposite of clean
code.


(I formerly worked at Infineon and  I don't think they're in a position
to complain that other people's code is "smelly"!)


J'


On Mon, Aug 13, 2018 at 06:25:38PM +0000, Christoph Hazott wrote:
     Hi,
     
     
     first of all I quickly want to introduce myself. My name is Christoph Hazott, I life in Austria but am originally from Germany. Currently I'm employed at Infineon as Test Development Engineer for RADAR Products and made my MSc. Degree in Embedded Systems Design at the University of Applied Sciences Upper Austria in 2012.
     
     I'm very fond of Free Software but have to say that due to (in my opinion) software entropy with such a high amount of contributors and functionality the tools have become smelly.
     
     
     I sat down now for one day and made a static code analysis of the binutils (because they are the major part of the toolchain). I uploaded the code of version 2.30.90 (last snapshot) to my public GitHub (https://github.com/h4z4rt/binutils) and connected it to sonar. In a virtual machine I executed the scanner with a Linux From Scratch configuration and the results where  uploaded to https://sonarcloud.io/organizations/h4z4rt-github/projects  and can be viewed there.
     
     
     
     The results show 535 Bugs, 804 Vulnerabilities, 8.5k Code Smells and > 11.7% duplications.
     
     
     
     Because of this I would like to contribute to the binutils project by setting up an infrastructure for static code analysis (and further...).
     
     I would be happy if you would liketo have me to contribute to the binutils like this.
     
     
     
     Looking forward to your supply.
     
     
     
     Regards,
     
     
     
     Christoph
     
     
     
     PS: I'm not fixed on any code analysis tools I just used sonar because I personally made the best experience with it.
     
     
     
     
     
     
     
     
     
     
     

--
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Reply | Threaded
Open this post in threaded view
|

AW: Software Quality Binutils

Christoph Hazott
Hi John,

first of all I'm sorry if I offended you, that was definitely not my intention!
I'm also not a spokesperson for Infineon. I'm writing you as a private person. The first paragraph was only to introduce myself to maybe get to know each other a little.

With "smelly" I was not referring to (sorry about the term) crap but to the term from Kent Beck "Code smell" (see Wiki article https://en.wikipedia.org/wiki/Code_smell)

Sonar in its default configuration I fully agree and it was just a quick check I created if I would be able to contribute like this (as written in the PS I'm not drawn to it).

Again sorry if my mail offended you!

Regards,

Christoph

-----Ursprüngliche Nachricht-----
Von: John Darrington [mailto:[hidden email]]
Gesendet: Monday, 13 August 2018 21:52
An: Christoph Hazott
Cc: [hidden email]
Betreff: Re: Software Quality Binutils

Hi Christoph,

I'm sure that if you identify any particular issues and send
patches, then they'll be carefully considered by the relevant person.

However I'm equally sure that if you send patches simply because they
shut sonar up then most will be rejected.   Sonar in its default
cofiguration is highly promiscuous - it includes criteria from
committees such as MISRA who in my opionion have failed to grasp many
principles of coding, and their rules achive the opposite of clean
code.


(I formerly worked at Infineon and  I don't think they're in a position
to complain that other people's code is "smelly"!)


J'


On Mon, Aug 13, 2018 at 06:25:38PM +0000, Christoph Hazott wrote:
     Hi,
     
     
     first of all I quickly want to introduce myself. My name is Christoph Hazott, I life in Austria but am originally from Germany. Currently I'm employed at Infineon as Test Development Engineer for RADAR Products and made my MSc. Degree in Embedded Systems Design at the University of Applied Sciences Upper Austria in 2012.
     
     I'm very fond of Free Software but have to say that due to (in my opinion) software entropy with such a high amount of contributors and functionality the tools have become smelly.
     
     
     I sat down now for one day and made a static code analysis of the binutils (because they are the major part of the toolchain). I uploaded the code of version 2.30.90 (last snapshot) to my public GitHub (https://github.com/h4z4rt/binutils) and connected it to sonar. In a virtual machine I executed the scanner with a Linux From Scratch configuration and the results where  uploaded to https://sonarcloud.io/organizations/h4z4rt-github/projects  and can be viewed there.
     
     
     
     The results show 535 Bugs, 804 Vulnerabilities, 8.5k Code Smells and > 11.7% duplications.
     
     
     
     Because of this I would like to contribute to the binutils project by setting up an infrastructure for static code analysis (and further...).
     
     I would be happy if you would liketo have me to contribute to the binutils like this.
     
     
     
     Looking forward to your supply.
     
     
     
     Regards,
     
     
     
     Christoph
     
     
     
     PS: I'm not fixed on any code analysis tools I just used sonar because I personally made the best experience with it.
     
     
     
     
     
     
     
     
     
     
     

--
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Reply | Threaded
Open this post in threaded view
|

Re: Software Quality Binutils

John Darrington-4
Hi Christoph,

I'm not at all offended.

I'm relatively new to binutils development, and my contributions are
limited to a particular architecture.   The binutils maintainer (Nick
Clifton) is responsible for high level decisions.

But personally I would welcome having some regular, systematic and automated
static *and* dynamic tests on binutils with publically available
results.  This would help alleviate some regressions which someone complained
about recently.

Binutils is mature software, and some parts of it are anachronistic, and
IMO should be rewritten.   For example I'm not impressed with the way
some .c files are #included by other files and their semantics adjusted
by the preprocessor.   But of course it's a question of time and
resources.   Static analysis could help identify which parts are in most
urgent need of attention.

As for tools, I've had good experience with cppcheck for linting
and with pmccabe for cyclomatic complexity.
[ How about gas/config/tc-hppc.c (pa_ip) with a complexity of 499 ?
  Now that is smelly! ]

You should also bear in mind that many source files in the git repository are
automatically generated (something which irks me immensely) and you
can't expect static analysis tools to give meaningfull results on those.

But these are just my opinions and I'm sure there are people on this
list who would consider them BS.

J'

On Mon, Aug 13, 2018 at 08:17:58PM +0000, Christoph Hazott wrote:
     Hi John,
     
     first of all I'm sorry if I offended you, that was definitely not my intention!
     I'm also not a spokesperson for Infineon. I'm writing you as a private person. The first paragraph was only to introduce myself to maybe get to know each other a little.
     
     With "smelly" I was not referring to (sorry about the term) crap but to the term from Kent Beck "Code smell" (see Wiki article https://en.wikipedia.org/wiki/Code_smell)
     
     Sonar in its default configuration I fully agree and it was just a quick check I created if I would be able to contribute like this (as written in the PS I'm not drawn to it).
     
     Again sorry if my mail offended you!
     
     Regards,
     
     Christoph
     
     -----Urspr?ngliche Nachricht-----
     Von: John Darrington [mailto:[hidden email]]
     Gesendet: Monday, 13 August 2018 21:52
     An: Christoph Hazott
     Cc: [hidden email]
     Betreff: Re: Software Quality Binutils
     
     Hi Christoph,
     
     I'm sure that if you identify any particular issues and send
     patches, then they'll be carefully considered by the relevant person.
     
     However I'm equally sure that if you send patches simply because they
     shut sonar up then most will be rejected.   Sonar in its default
     cofiguration is highly promiscuous - it includes criteria from
     committees such as MISRA who in my opionion have failed to grasp many
     principles of coding, and their rules achive the opposite of clean
     code.
     
     
     (I formerly worked at Infineon and  I don't think they're in a position
     to complain that other people's code is "smelly"!)
     
     
     J'
     
     
     On Mon, Aug 13, 2018 at 06:25:38PM +0000, Christoph Hazott wrote:
          Hi,
         
         
          first of all I quickly want to introduce myself. My name is Christoph Hazott, I life in Austria but am originally from Germany. Currently I'm employed at Infineon as Test Development Engineer for RADAR Products and made my MSc. Degree in Embedded Systems Design at the University of Applied Sciences Upper Austria in 2012.
         
          I'm very fond of Free Software but have to say that due to (in my opinion) software entropy with such a high amount of contributors and functionality the tools have become smelly.
         
         
          I sat down now for one day and made a static code analysis of the binutils (because they are the major part of the toolchain). I uploaded the code of version 2.30.90 (last snapshot) to my public GitHub (https://github.com/h4z4rt/binutils) and connected it to sonar. In a virtual machine I executed the scanner with a Linux From Scratch configuration and the results where  uploaded to https://sonarcloud.io/organizations/h4z4rt-github/projects  and can be viewed there.
         
         
         
          The results show 535 Bugs, 804 Vulnerabilities, 8.5k Code Smells and > 11.7% duplications.
         
         
         
          Because of this I would like to contribute to the binutils project by setting up an infrastructure for static code analysis (and further...).
         
          I would be happy if you would liketo have me to contribute to the binutils like this.
         
         
         
          Looking forward to your supply.
         
         
         
          Regards,
         
         
         
          Christoph
         
         
         
          PS: I'm not fixed on any code analysis tools I just used sonar because I personally made the best experience with it.
         
         
         
         
         
         
         
         
         
         
         
     
     --
     Avoid eavesdropping.  Send strong encrypted email.
     PGP Public key ID: 1024D/2DE827B3
     fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
     See http://sks-keyservers.net or any PGP keyserver for public key.
     

--
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Reply | Threaded
Open this post in threaded view
|

Re: Software Quality Binutils

Nick Clifton
In reply to this post by Christoph Hazott
Hi Christoph,

> In a virtual machine I executed the scanner with a Linux From Scratch
> configuration and the results where  uploaded to
> https://sonarcloud.io/organizations/h4z4rt-github/projects and can be
> viewed there.

Thanks very much for taking an interest in the binutils, and for letting
us know about your scan and its results.  If there are any serious bugs
that are uncovered it would be really useful if they could be reported
via the binutils bug tracking system:

  https://sourceware.org/bugzilla/enter_bug.cgi?product=binutils

I took a quick look at the scan results myself.  535 bugs does seem to
be rather alarming.  To say nothing of the vulnerabilities and smells.
But when I took a look at some individual bugs I have to say that I was
not very impressed.  Comments like "review this data-flow, variable
<foo> may be null" indicate to me that the tool is not performing an
in-depth analysis of the code.

Or "Remove this conditional structure or edit its code blocks so that
they're not all the same".  How on earth is that a bug ?  It is not
even bad coding.

I apologise, because I have not been through every single bug report
to see if any of them are significant.  But with that much noise in
the output I doubt if anyone will go through all of those "bugs".
Is there any way to adjust the output of the scanner so that only
really significant bugs are reported ?

Cheers
  Nick
Reply | Threaded
Open this post in threaded view
|

Re: Software Quality Binutils

John Darrington-4
On Tue, Aug 14, 2018 at 11:31:24AM +0100, Nick Clifton wrote:
     
     I took a quick look at the scan results myself.  535 bugs does seem to
     be rather alarming.  To say nothing of the vulnerabilities and smells.
     But when I took a look at some individual bugs I have to say that I was
     not very impressed.  Comments like "review this data-flow, variable
     <foo> may be null" indicate to me that the tool is not performing an
     in-depth analysis of the code.

It would take an artificial intellegence engine to know that in the code

struct foo x = NULL;
x = allocate_foo ();
x->member = 1;

allocate_foo might well guarantee never to return NULL.   So some
"authorities" recommand *always* checking x before dereferencing it.
I don't subscribe to that point of view.   But include/ansidecl.h
has ATTRIBUTE_RETURNS_NONNULL, which static analysis tools could use
(but so far as I'm aware, none do).  Nevertheless, I think binutils
could make more use of these gcc attributes than it currently does.

     
     Or "Remove this conditional structure or edit its code blocks so that
     they're not all the same".  How on earth is that a bug ?  It is not
     even bad coding.

I think it's flagging things like:

if (cond)
{
  do_this ();
  do_that ();
}
else
{
  do_this ();
}

... which it wants changed to:

do_this ();
if (cond)
 do_that ();


which of-course has the same effect iff evaluating cond does not
have side effects.

J'


--
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Reply | Threaded
Open this post in threaded view
|

Re: Software Quality Binutils

Nick Clifton
Hi John,

> But include/ansidecl.h
> has ATTRIBUTE_RETURNS_NONNULL, which static analysis tools could use
> (but so far as I'm aware, none do).  Nevertheless, I think binutils
> could make more use of these gcc attributes than it currently does.

Agreed.  And I will be happy to review any patches to add such attributes...

>      Or "Remove this conditional structure or edit its code blocks so that
>      they're not all the same".  How on earth is that a bug ?  It is not
>      even bad coding.
>
> I think it's flagging things like:
>
> if (cond)
> {
>   do_this ();
>   do_that ();
> }
> else
> {
>   do_this ();
> }
>
> ... which it wants changed to:
>
> do_this ();
> if (cond)
>  do_that ();
>
>
> which of-course has the same effect iff evaluating cond does not
> have side effects.

True, but I do not see this as being a bug.  A possible situation
for code quality improvement maybe, but not a bug.

Besides, how do I tell this scanner that actually the code sequence
is fine as it is and does not need to be changed ?  I am not going
to change the code just to pacify a scanner.  (I know that you said
the same thing - I am agreeing with you - so this comment is really
directed to Christoph).

Cheers
  Nick
Reply | Threaded
Open this post in threaded view
|

Re: Software Quality Binutils

Paul Koning-6
In reply to this post by John Darrington-4


> On Aug 14, 2018, at 7:12 AM, John Darrington <[hidden email]> wrote:
>
> ...
> I think it's flagging things like:
>
> if (cond)
> {
>  do_this ();
>  do_that ();
> }
> else
> {
>  do_this ();
> }
>
> ... which it wants changed to:
>
> do_this ();
> if (cond)
> do_that ();
>
>
> which of-course has the same effect iff evaluating cond does not
> have side effects.

Not so; if do_this() may change cond, then the two are semantically different.

        paul

Reply | Threaded
Open this post in threaded view
|

AW: Software Quality Binutils

Christoph Hazott
Hi to all,

I'm very happy because I think we are at the same page here :-)
@John: CppCheck and pmccabe look already quite promising and could be a much better alternative than the sonar check I did. The pa_ip function is awesome I have to say :-D ~2000LOC with almost all loops and branches is a real challenge to maintain and much more of a challenge to refactor properly to not miss anything!
@Nick: The check with sonar was done with the cx11 code rules from sonar. Also I don't like the bug term in sonar because a bug is something that is defined to lead to faults. An I personally think that this can be indicated by a static code analysis tool but real proof you only get over a dynamic code analysis (like unit testing). And to answer your questions. If "we" do static code analysis on the binutils we would need to have to define a set of rules and tools that really apply and make sense to further progress. The function John mentioned is a good example. If we assume that every node in the program is binary the CC tells us that 2^499 unit tests will cover this function. E.g. refactoring this to have 10 tests with a complexity of ~49 means we need around 10*2^49 tests to reach the same coverage. That's just a made up example but that's the direction where static code analysis is aiming.
@Paul: That's true and from my experience the graph system in the background of the tools I used so far was always able to identify the proper case.

Maybe it would be an idea that we pick out a module or function as a starting point which I then can work through to continue this conversation on a real example? The pa_ip function would already be quite an awesome challenge for this?

What do you think?

Regards,

Christoph


-----Ursprüngliche Nachricht-----
Von: Paul Koning [mailto:[hidden email]]
Gesendet: Tuesday, 14 August 2018 15:22
An: John Darrington
Cc: Nick Clifton; Christoph Hazott; [hidden email]
Betreff: Re: Software Quality Binutils



> On Aug 14, 2018, at 7:12 AM, John Darrington <[hidden email]> wrote:
>
> ...
> I think it's flagging things like:
>
> if (cond)
> {
>  do_this ();
>  do_that ();
> }
> else
> {
>  do_this ();
> }
>
> ... which it wants changed to:
>
> do_this ();
> if (cond)
> do_that ();
>
>
> which of-course has the same effect iff evaluating cond does not
> have side effects.

Not so; if do_this() may change cond, then the two are semantically different.

        paul

Reply | Threaded
Open this post in threaded view
|

Re: AW: Software Quality Binutils

Jeff Law
On 08/16/2018 10:10 AM, Christoph Hazott wrote:

> Hi to all,
>
> I'm very happy because I think we are at the same page here :-)
> @John: CppCheck and pmccabe look already quite promising and could be
> a much better alternative than the sonar check I did. The pa_ip
> function is awesome I have to say :-D ~2000LOC with almost all loops
> and branches is a real challenge to maintain and much more of a
> challenge to refactor properly to not miss anything! @Nick: The check
> with sonar was done with the cx11 code rules from sonar. Also I don't
> like the bug term in sonar because a bug is something that is defined
> to lead to faults. An I personally think that this can be indicated
> by a static code analysis tool but real proof you only get over a
> dynamic code analysis (like unit testing). And to answer your
> questions. If "we" do static code analysis on the binutils we would
> need to have to define a set of rules and tools that really apply and
> make sense to further progress. The function John mentioned is a good
> example. If we assume that every node in the program is binary the CC
> tells us that 2^499 unit tests will cover this function. E.g.
> refactoring this to have 10 tests with a complexity of ~49 means we
> need around 10*2^49 tests to reach the same coverage. That's just a
> made up example but that's the direction where static code analysis
> is aiming. @Paul: That's true and from my experience the graph system
> in the background of the tools I used so far was always able to
> identify the proper case.
>
> Maybe it would be an idea that we pick out a module or function as a
> starting point which I then can work through to continue this
> conversation on a real example? The pa_ip function would already be
> quite an awesome challenge for this?
Rather than focusing on a dead target (HPPA), I'd suggest focusing on
something more mainstream if you're trying to improve code quality.  You
could spend enormous amount of time bullet proofing code nobody runs
anymore in the real world.

jeff
Reply | Threaded
Open this post in threaded view
|

AW: AW: Software Quality Binutils

Christoph Hazott
Hi Jeff,

yes of course! Do you have any suggestions?

Regards,

Christoph

-----Ursprüngliche Nachricht-----
Von: Jeff Law [mailto:[hidden email]]
Gesendet: Thursday, 16 August 2018 18:13
An: Christoph Hazott; Paul Koning; John Darrington; Nick Clifton
Cc: [hidden email]
Betreff: Re: AW: Software Quality Binutils

On 08/16/2018 10:10 AM, Christoph Hazott wrote:

> Hi to all,
>
> I'm very happy because I think we are at the same page here :-)
> @John: CppCheck and pmccabe look already quite promising and could be
> a much better alternative than the sonar check I did. The pa_ip
> function is awesome I have to say :-D ~2000LOC with almost all loops
> and branches is a real challenge to maintain and much more of a
> challenge to refactor properly to not miss anything! @Nick: The check
> with sonar was done with the cx11 code rules from sonar. Also I don't
> like the bug term in sonar because a bug is something that is defined
> to lead to faults. An I personally think that this can be indicated
> by a static code analysis tool but real proof you only get over a
> dynamic code analysis (like unit testing). And to answer your
> questions. If "we" do static code analysis on the binutils we would
> need to have to define a set of rules and tools that really apply and
> make sense to further progress. The function John mentioned is a good
> example. If we assume that every node in the program is binary the CC
> tells us that 2^499 unit tests will cover this function. E.g.
> refactoring this to have 10 tests with a complexity of ~49 means we
> need around 10*2^49 tests to reach the same coverage. That's just a
> made up example but that's the direction where static code analysis
> is aiming. @Paul: That's true and from my experience the graph system
> in the background of the tools I used so far was always able to
> identify the proper case.
>
> Maybe it would be an idea that we pick out a module or function as a
> starting point which I then can work through to continue this
> conversation on a real example? The pa_ip function would already be
> quite an awesome challenge for this?
Rather than focusing on a dead target (HPPA), I'd suggest focusing on
something more mainstream if you're trying to improve code quality.  You
could spend enormous amount of time bullet proofing code nobody runs
anymore in the real world.

jeff
Reply | Threaded
Open this post in threaded view
|

Re: AW: AW: Software Quality Binutils

Nick Clifton
Hi Christoph,

> yes of course! Do you have any suggestions?

The x86_64 target would be an obvious first choice.  It has lots
of active maintainers and users.

Cheers
  Nick


Reply | Threaded
Open this post in threaded view
|

Re: Software Quality Binutils

Joel Sherrill
In reply to this post by Paul Koning-6
On Tue, Aug 14, 2018 at 8:22 AM, Paul Koning <[hidden email]> wrote:

>
>
> > On Aug 14, 2018, at 7:12 AM, John Darrington <
> [hidden email]> wrote:
> >
> > ...
> > I think it's flagging things like:
> >
> > if (cond)
> > {
> >  do_this ();
> >  do_that ();
> > }
> > else
> > {
> >  do_this ();
> > }
> >
> > ... which it wants changed to:
> >
> > do_this ();
> > if (cond)
> > do_that ();
> >
> >
> > which of-course has the same effect iff evaluating cond does not
> > have side effects.
>
> Not so; if do_this() may change cond, then the two are semantically
> different.
>

When looking at static analyzer output for RTEMS, we took places like this
as a suggestion that either this change is a nice idea or we need a comment
to indicate that cond could actually change as a side-effect. We don't
someone
to be tempted if it is dangerous or wrong.

Is this Sonar the same as GrammaTech CodeSonar?

--joel


>
>         paul
>
>
Reply | Threaded
Open this post in threaded view
|

Re: AW: Software Quality Binutils

John Darrington-4
In reply to this post by Christoph Hazott
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
[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'.
[bfd/elf32-rl78.c:476]: (error) Division by zero.
[bfd/elf32-rl78.c:532]: (error) Division 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
[bfd/elf64-alpha.c:5041]: (error) Shifting a negative value is undefined behaviour
[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'.
[bfd/gen-aout.c:54]: (error) Resource leak: file
[bfd/mach-o.c:5582]: (error) Signed integer overflow for expression '3221225472-67108864'.
[bfd/mmo.c:497]: (error) Buffer is accessed out of bounds: valid_mmo_symbol_character_set
[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'.


Some of those seem a little concerning at first glance  ...

J'

--
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Reply | Threaded
Open this post in threaded view
|

Re: AW: Software Quality Binutils

Alan Modra-3
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
Reply | Threaded
Open this post in threaded view
|

Re: AW: Software Quality Binutils

John Darrington-4
In reply to this post by John Darrington-4
I looked deeper at some of the cases that cppcheck threw up.   Some are
clearly false, others I cannot judge, but this one

  [elf64-hppa.c:3575] -> [elf64-hppa.c:3561]: (warning) Either the condition hh==0 is redundant or there is possible null pointer dereference: hh.

unless I am very much mistaken is guaanteed to crash if the relevant
line is ever reached.

J'


--
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Reply | Threaded
Open this post in threaded view
|

Re: AW: Software Quality Binutils

Nick Clifton
Hi John,

> I looked deeper at some of the cases that cppcheck threw up.   Some are
> clearly false, others I cannot judge, but this one
>
>   [elf64-hppa.c:3575] -> [elf64-hppa.c:3561]: (warning) Either the condition hh==0 is redundant or there is possible null pointer dereference: hh.
>
> unless I am very much mistaken is guaanteed to crash if the relevant
> line is ever reached.

Indeed you are right.  I am checking the patch below to remove the
block of code, since it can never have been executed, and replace
it with an assertion.

Cheers
  Nick

bfd/ChangeLog
2018-08-23  Nick Clifton  <[hidden email]>

        * elf64-hppa.c (elf_hppa_final_link_relocate): Replace unworkable
        code with an assertion.

diff --git a/bfd/elf64-hppa.c b/bfd/elf64-hppa.c
index 2e66c92bfb..b4f047fa64 100644
--- a/bfd/elf64-hppa.c
+++ b/bfd/elf64-hppa.c
@@ -3556,33 +3556,12 @@ elf_hppa_final_link_relocate (Elf_Internal_Rela *rel,
 
     case R_PARISC_LTOFF_FPTR32:
       {
- /* We may still need to create the FPTR itself if it was for
-   a local symbol.  */
- if (hh == NULL)
-  {
-    /* The first two words of an .opd entry are zero.  */
-    memset (hppa_info->opd_sec->contents + hh->opd_offset, 0, 16);
-
-    /* The next word is the address of the function.  */
-    bfd_put_64 (hppa_info->opd_sec->owner, value + addend,
- (hppa_info->opd_sec->contents
- + hh->opd_offset + 16));
-
-    /* The last word is our local __gp value.  */
-    value = _bfd_get_gp_value
-      (hppa_info->opd_sec->output_section->owner);
-    bfd_put_64 (hppa_info->opd_sec->owner, value,
- hppa_info->opd_sec->contents + hh->opd_offset + 24);
-
-    /* The DLT value is the address of the .opd entry.  */
-    value = (hh->opd_offset
-     + hppa_info->opd_sec->output_offset
-     + hppa_info->opd_sec->output_section->vma);
-
-    bfd_put_64 (hppa_info->dlt_sec->owner,
- value,
- hppa_info->dlt_sec->contents + hh->dlt_offset);
-  }
+ /* FIXME: There used to be code here to create the FPTR itself if
+   the relocation was against a local symbol.  But the code could
+   never have worked.  If the assert below is ever triggered then
+   the code will need to be reinstated and fixed so that it does
+   what is needed.  */
+ BFD_ASSERT (hh != NULL);
 
  /* We want the value of the DLT offset for this symbol, not
    the symbol's actual address.  Note that __gp may not point
Reply | Threaded
Open this post in threaded view
|

Re: AW: Software Quality Binutils

Jeff Law
On 08/23/2018 06:34 AM, Nick Clifton wrote:

> Hi John,
>
>> I looked deeper at some of the cases that cppcheck threw up.   Some are
>> clearly false, others I cannot judge, but this one
>>
>>   [elf64-hppa.c:3575] -> [elf64-hppa.c:3561]: (warning) Either the condition hh==0 is redundant or there is possible null pointer dereference: hh.
>>
>> unless I am very much mistaken is guaanteed to crash if the relevant
>> line is ever reached.
>
> Indeed you are right.  I am checking the patch below to remove the
> block of code, since it can never have been executed, and replace
> it with an assertion.
>
> Cheers
>   Nick
>
> bfd/ChangeLog
> 2018-08-23  Nick Clifton  <[hidden email]>
>
> * elf64-hppa.c (elf_hppa_final_link_relocate): Replace unworkable
> code with an assertion.
>
> diff --git a/bfd/elf64-hppa.c b/bfd/elf64-hppa.c
> index 2e66c92bfb..b4f047fa64 100644
> --- a/bfd/elf64-hppa.c
> +++ b/bfd/elf64-hppa.c
> @@ -3556,33 +3556,12 @@ elf_hppa_final_link_relocate (Elf_Internal_Rela *rel,
>  
>      case R_PARISC_LTOFF_FPTR32:
>        {
> - /* We may still need to create the FPTR itself if it was for
> -   a local symbol.  */
> - if (hh == NULL)
> -  {
> -    /* The first two words of an .opd entry are zero.  */
> -    memset (hppa_info->opd_sec->contents + hh->opd_offset, 0, 16);
> -
> -    /* The next word is the address of the function.  */
> -    bfd_put_64 (hppa_info->opd_sec->owner, value + addend,
> - (hppa_info->opd_sec->contents
> - + hh->opd_offset + 16));
> -
> -    /* The last word is our local __gp value.  */
> -    value = _bfd_get_gp_value
> -      (hppa_info->opd_sec->output_section->owner);
> -    bfd_put_64 (hppa_info->opd_sec->owner, value,
> - hppa_info->opd_sec->contents + hh->opd_offset + 24);
> -
> -    /* The DLT value is the address of the .opd entry.  */
> -    value = (hh->opd_offset
> -     + hppa_info->opd_sec->output_offset
> -     + hppa_info->opd_sec->output_section->vma);
> -
> -    bfd_put_64 (hppa_info->dlt_sec->owner,
> - value,
> - hppa_info->dlt_sec->contents + hh->dlt_offset);
> -  }
> + /* FIXME: There used to be code here to create the FPTR itself if
> +   the relocation was against a local symbol.  But the code could
> +   never have worked.  If the assert below is ever triggered then
> +   the code will need to be reinstated and fixed so that it does
> +   what is needed.  */
> + BFD_ASSERT (hh != NULL);
My involvement with the PA has diminished greatly, but I think this code
is to support indirect calls to local functions.  The big question is
did the 32-bit ELF ABI eliminate the need for fptr relocations -- I
vaguely recall they wanted to clean this up because the cost of indirect
calls was considered high, but I simply don't remember if it changed.

The way to try and trigger would be to have an indirect call to a local
function.   At least with the SOM 32bit ABI that should trigger the need
for an FPTR that references a local function.

Jeff