Message ID | 20140917195918.GA6046@f1.c.bardezibar.internal |
---|---|
State | New |
Headers | show |
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: /* 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. Jeff
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
> 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. I am bit in hurry, so just quickly (will give it second look at evening). The pass is a simple dataflow propagating preferred sections. NULL means that the symbol in question was not visited yet. Assuming that all symbols are reachable, this should not happen by the end of dataflow process (well, thining about it, perhaps used attribute can trigger it, but it ought to be handled earlier). So I would like to know why the symbols survives remove_unreachable_functions... Honza > > Sebastian
On 09/19/14 08:11, Sebastian Pop wrote: > 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. Certainly attach it to a BZ -- from Jan's message it sounds like a NULL in that spot shouldn't be happening. So I suspect he'll want to take a deeper look. jeff
Jan Hubicka wrote: > I am bit in hurry, so just quickly (will give it second look at evening). The > pass is a simple dataflow propagating preferred sections. NULL means that the > symbol in question was not visited yet. Assuming that all symbols are reachable, > this should not happen by the end of dataflow process (well, thining about it, > perhaps used attribute can trigger it, but it ought to be handled earlier). > > So I would like to know why the symbols survives remove_unreachable_functions... https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63313 Let me know if you cannot reproduce the ICE. Thanks, Sebastian
diff --git a/gcc/ipa-comdats.c b/gcc/ipa-comdats.c index b270d97..57e8239 100644 --- a/gcc/ipa-comdats.c +++ b/gcc/ipa-comdats.c @@ -317,7 +317,13 @@ ipa_comdats (void) && !symbol->alias && symbol->real_symbol_p ()) { - tree group = *map.get (symbol); + tree group = NULL; + /* Get current lattice value of SYMBOL. */ + tree *val = map.get (symbol); + if (val) + group = *val; + else + continue; if (group == error_mark_node) continue;
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? Thanks, Sebastian From f5934ecdee5d7e8e143310c21906d5099b9e7d23 Mon Sep 17 00:00:00 2001 From: Sebastian Pop <s.pop@samsung.com> Date: Wed, 17 Sep 2014 14:04:20 -0500 Subject: [PATCH] avoid dereferencing a NULL pointer 2014-09-17 Sebastian Pop <s.pop@samsung.com> * ipa-comdats.c (ipa_comdats): Check that the value in the map is non NULL before dereferencing it. --- gcc/ipa-comdats.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)