Re: [patch] avoid ICE due to NULL pointer dereference in ipa-comdats.c

[Sebastian Pop <sebpop at gmail dot com>]

Jeff Law wrote:
> On 09/17/14 13:59, Sebastian Pop wrote:
> >Hi,
> >
> >I got an ICE while building libstdc++ of a cross compiler x86 to aarch64.  I
> >have a testcase that ICEs on current GCC trunk.  I was trying to painfully
> >reduce it with creduce, and it is still several thousand lines of c++. Frustrated
> >that it does not reduce anymore, I decided to have a look with gdb at why the
> >compiler was iceing: the code dereferences a NULL pointer that we get by looking
> >up the value of a symbol in a map.  Around that place, there is another pattern
> >that first makes sure that the pointer we get from the map is non NULL: I copied
> >that code around and that seemed to have solved the ICE.
> >
> >Regtested on x86-64-linux, and also checked that my aarch64 cross compilers are
> >now building correctly libstdc++.
> >
> >Ok to commit?
> Well, the obvious question is whether or not we ought to be able to
> lookup the symbol in that particular map.
> 
> You don't mention the other similar pattern of code which checks for
> a NULL return value from map.get, so I had to guess it was this one
> just above the one you want to change:

Right.

> 
>      /* Get current lattice value of SYMBOL.  */
>       val = map.get (symbol);
>       if (val)
>         group = *val;
> 
>       /* If it is bottom, there is nothing to do; do not clear AUX
>          so we won't re-queue the symbol.  */
>       if (group == error_mark_node)
>         continue;
> 
> I couldn't convince myself this code was correct if map.get returns
> a NULL pointer either!  It certainly isn't obvious if just
> continuing either loop is correct if the symbol isn't found.
> 
> I really think Jan needs to chime in here.

If there is need for a testcase, I will open a bug report and attach the
preprocessed file that causes the ICE.  As I said it is still a pretty large
file, and creduce was not able to reduce it further.

Sebastian