Message ID | 1532392160-4447-1-git-send-email-dmalcolm@redhat.com |
---|---|
State | New |
Headers | show |
Series | Fix segfault in -fsave-optimization-record (PR tree-optimization/86636) | expand |
On Tue, Jul 24, 2018 at 1:44 AM David Malcolm <dmalcolm@redhat.com> wrote: > > There are various ways that it's possible for a gimple statement to > have an UNKNOWN_LOCATION, and for that UNKNOWN_LOCATION to be wrapped > in an ad-hoc location to capture inlining information. > > For such a location, LOCATION_FILE (loc) is NULL. > > Various places in -fsave-optimization-record were checking for > loc != UNKNOWN_LOCATION > and were passing LOCATION_FILE (loc) to code that assumed a non-NULL > filename, thus leading to segfaults for the above cases. > > This patch updates the tests to use > LOCATION_LOCUS (loc) != UNKNOWN_LOCATION > instead, to look through ad-hoc location wrappers, fixing the segfaults. > > It also adds various assertions to the affected code. > > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu; adds > 8 PASS results to gcc.sum. > > OK for trunk? OK. Richard. > > gcc/ChangeLog: > PR tree-optimization/86636 > * json.cc (json::object::set): Fix comment. Add assertions. > (json::array::append): Move here from json.h. Add comment and an > assertion. > (json::string::string): Likewise. > * json.h (json::array::append): Move to json.cc. > (json::string::string): Likewise. > * optinfo-emit-json.cc > (optrecord_json_writer::impl_location_to_json): Assert that we > aren't attempting to write out UNKNOWN_LOCATION, or an ad-hoc > wrapper around it. Expand the location once, rather than three > times. > (optrecord_json_writer::inlining_chain_to_json): Fix the check for > UNKNOWN_LOCATION, to use LOCATION_LOCUS to look through ad-hoc > wrappers. > (optrecord_json_writer::optinfo_to_json): Likewise, in four > places. Fix some overlong lines. > > gcc/testsuite/ChangeLog: > PR tree-optimization/86636 > * gcc.c-torture/compile/pr86636.c: New test. > --- > gcc/json.cc | 24 +++++++++++++++++++++++- > gcc/json.h | 4 ++-- > gcc/optinfo-emit-json.cc | 25 +++++++++++++++---------- > gcc/testsuite/gcc.c-torture/compile/pr86636.c | 8 ++++++++ > 4 files changed, 48 insertions(+), 13 deletions(-) > create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr86636.c > > diff --git a/gcc/json.cc b/gcc/json.cc > index 3c2aa77..3ead980 100644 > --- a/gcc/json.cc > +++ b/gcc/json.cc > @@ -76,12 +76,15 @@ object::print (pretty_printer *pp) const > pp_character (pp, '}'); > } > > -/* Set the json::value * for KEY, taking ownership of VALUE > +/* Set the json::value * for KEY, taking ownership of V > (and taking a copy of KEY if necessary). */ > > void > object::set (const char *key, value *v) > { > + gcc_assert (key); > + gcc_assert (v); > + > value **ptr = m_map.get (key); > if (ptr) > { > @@ -126,6 +129,15 @@ array::print (pretty_printer *pp) const > pp_character (pp, ']'); > } > > +/* Append non-NULL value V to a json::array, taking ownership of V. */ > + > +void > +array::append (value *v) > +{ > + gcc_assert (v); > + m_elements.safe_push (v); > +} > + > /* class json::number, a subclass of json::value, wrapping a double. */ > > /* Implementation of json::value::print for json::number. */ > @@ -140,6 +152,16 @@ number::print (pretty_printer *pp) const > > /* class json::string, a subclass of json::value. */ > > +/* json::string's ctor. */ > + > +string::string (const char *utf8) > +{ > + gcc_assert (utf8); > + m_utf8 = xstrdup (utf8); > +} > + > +/* Implementation of json::value::print for json::string. */ > + > void > string::print (pretty_printer *pp) const > { > diff --git a/gcc/json.h b/gcc/json.h > index 5c3274c..154d9e1 100644 > --- a/gcc/json.h > +++ b/gcc/json.h > @@ -107,7 +107,7 @@ class array : public value > enum kind get_kind () const FINAL OVERRIDE { return JSON_ARRAY; } > void print (pretty_printer *pp) const FINAL OVERRIDE; > > - void append (value *v) { m_elements.safe_push (v); } > + void append (value *v); > > private: > auto_vec<value *> m_elements; > @@ -134,7 +134,7 @@ class number : public value > class string : public value > { > public: > - string (const char *utf8) : m_utf8 (xstrdup (utf8)) {} > + string (const char *utf8); > ~string () { free (m_utf8); } > > enum kind get_kind () const FINAL OVERRIDE { return JSON_STRING; } > diff --git a/gcc/optinfo-emit-json.cc b/gcc/optinfo-emit-json.cc > index bf1172a..6460a81 100644 > --- a/gcc/optinfo-emit-json.cc > +++ b/gcc/optinfo-emit-json.cc > @@ -202,10 +202,12 @@ optrecord_json_writer::impl_location_to_json (dump_impl_location_t loc) > json::object * > optrecord_json_writer::location_to_json (location_t loc) > { > + gcc_assert (LOCATION_LOCUS (loc) != UNKNOWN_LOCATION); > + expanded_location exploc = expand_location (loc); > json::object *obj = new json::object (); > - obj->set ("file", new json::string (LOCATION_FILE (loc))); > - obj->set ("line", new json::number (LOCATION_LINE (loc))); > - obj->set ("column", new json::number (LOCATION_COLUMN (loc))); > + obj->set ("file", new json::string (exploc.file)); > + obj->set ("line", new json::number (exploc.line)); > + obj->set ("column", new json::number (exploc.column)); > return obj; > } > > @@ -330,7 +332,7 @@ optrecord_json_writer::inlining_chain_to_json (location_t loc) > const char *printable_name > = lang_hooks.decl_printable_name (fndecl, 2); > obj->set ("fndecl", new json::string (printable_name)); > - if (*locus != UNKNOWN_LOCATION) > + if (LOCATION_LOCUS (*locus) != UNKNOWN_LOCATION) > obj->set ("site", location_to_json (*locus)); > array->append (obj); > } > @@ -371,8 +373,9 @@ optrecord_json_writer::optinfo_to_json (const optinfo *optinfo) > json_item->set ("expr", new json::string (item->get_text ())); > > /* Capture any location for the node. */ > - if (item->get_location () != UNKNOWN_LOCATION) > - json_item->set ("location", location_to_json (item->get_location ())); > + if (LOCATION_LOCUS (item->get_location ()) != UNKNOWN_LOCATION) > + json_item->set ("location", > + location_to_json (item->get_location ())); > > message->append (json_item); > } > @@ -383,8 +386,9 @@ optrecord_json_writer::optinfo_to_json (const optinfo *optinfo) > json_item->set ("stmt", new json::string (item->get_text ())); > > /* Capture any location for the stmt. */ > - if (item->get_location () != UNKNOWN_LOCATION) > - json_item->set ("location", location_to_json (item->get_location ())); > + if (LOCATION_LOCUS (item->get_location ()) != UNKNOWN_LOCATION) > + json_item->set ("location", > + location_to_json (item->get_location ())); > > message->append (json_item); > } > @@ -395,8 +399,9 @@ optrecord_json_writer::optinfo_to_json (const optinfo *optinfo) > json_item->set ("symtab_node", new json::string (item->get_text ())); > > /* Capture any location for the node. */ > - if (item->get_location () != UNKNOWN_LOCATION) > - json_item->set ("location", location_to_json (item->get_location ())); > + if (LOCATION_LOCUS (item->get_location ()) != UNKNOWN_LOCATION) > + json_item->set ("location", > + location_to_json (item->get_location ())); > message->append (json_item); > } > break; > diff --git a/gcc/testsuite/gcc.c-torture/compile/pr86636.c b/gcc/testsuite/gcc.c-torture/compile/pr86636.c > new file mode 100644 > index 0000000..2fe2f70 > --- /dev/null > +++ b/gcc/testsuite/gcc.c-torture/compile/pr86636.c > @@ -0,0 +1,8 @@ > +/* { dg-options "-fsave-optimization-record -ftree-loop-vectorize -ftree-parallelize-loops=2" } */ > + > +void > +n2 (int ih) > +{ > + while (ih < 1) > + ++ih; > +} > -- > 1.8.5.3 >
On 24/07/18 15:12, Richard Biener wrote: > On Tue, Jul 24, 2018 at 1:44 AM David Malcolm <dmalcolm@redhat.com> wrote: >> >> There are various ways that it's possible for a gimple statement to >> have an UNKNOWN_LOCATION, and for that UNKNOWN_LOCATION to be wrapped >> in an ad-hoc location to capture inlining information. >> >> For such a location, LOCATION_FILE (loc) is NULL. >> >> Various places in -fsave-optimization-record were checking for >> loc != UNKNOWN_LOCATION >> and were passing LOCATION_FILE (loc) to code that assumed a non-NULL >> filename, thus leading to segfaults for the above cases. >> >> This patch updates the tests to use >> LOCATION_LOCUS (loc) != UNKNOWN_LOCATION >> instead, to look through ad-hoc location wrappers, fixing the segfaults. >> >> It also adds various assertions to the affected code. >> >> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu; adds >> 8 PASS results to gcc.sum. >> >> OK for trunk? > > OK. > > Richard. > >> >> gcc/ChangeLog: >> PR tree-optimization/86636 >> * json.cc (json::object::set): Fix comment. Add assertions. >> (json::array::append): Move here from json.h. Add comment and an >> assertion. >> (json::string::string): Likewise. >> * json.h (json::array::append): Move to json.cc. >> (json::string::string): Likewise. >> * optinfo-emit-json.cc >> (optrecord_json_writer::impl_location_to_json): Assert that we >> aren't attempting to write out UNKNOWN_LOCATION, or an ad-hoc >> wrapper around it. Expand the location once, rather than three >> times. >> (optrecord_json_writer::inlining_chain_to_json): Fix the check for >> UNKNOWN_LOCATION, to use LOCATION_LOCUS to look through ad-hoc >> wrappers. >> (optrecord_json_writer::optinfo_to_json): Likewise, in four >> places. Fix some overlong lines. >> >> gcc/testsuite/ChangeLog: >> PR tree-optimization/86636 >> * gcc.c-torture/compile/pr86636.c: New test. >> --- >> gcc/json.cc | 24 +++++++++++++++++++++++- >> gcc/json.h | 4 ++-- >> gcc/optinfo-emit-json.cc | 25 +++++++++++++++---------- >> gcc/testsuite/gcc.c-torture/compile/pr86636.c | 8 ++++++++ >> 4 files changed, 48 insertions(+), 13 deletions(-) >> create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr86636.c >> >> diff --git a/gcc/json.cc b/gcc/json.cc >> index 3c2aa77..3ead980 100644 >> --- a/gcc/json.cc >> +++ b/gcc/json.cc >> @@ -76,12 +76,15 @@ object::print (pretty_printer *pp) const >> pp_character (pp, '}'); >> } >> >> -/* Set the json::value * for KEY, taking ownership of VALUE >> +/* Set the json::value * for KEY, taking ownership of V >> (and taking a copy of KEY if necessary). */ >> >> void >> object::set (const char *key, value *v) >> { >> + gcc_assert (key); >> + gcc_assert (v); >> + >> value **ptr = m_map.get (key); >> if (ptr) >> { >> @@ -126,6 +129,15 @@ array::print (pretty_printer *pp) const >> pp_character (pp, ']'); >> } >> >> +/* Append non-NULL value V to a json::array, taking ownership of V. */ >> + >> +void >> +array::append (value *v) >> +{ >> + gcc_assert (v); >> + m_elements.safe_push (v); >> +} >> + >> /* class json::number, a subclass of json::value, wrapping a double. */ >> >> /* Implementation of json::value::print for json::number. */ >> @@ -140,6 +152,16 @@ number::print (pretty_printer *pp) const >> >> /* class json::string, a subclass of json::value. */ >> >> +/* json::string's ctor. */ >> + >> +string::string (const char *utf8) >> +{ >> + gcc_assert (utf8); >> + m_utf8 = xstrdup (utf8); >> +} >> + >> +/* Implementation of json::value::print for json::string. */ >> + >> void >> string::print (pretty_printer *pp) const >> { >> diff --git a/gcc/json.h b/gcc/json.h >> index 5c3274c..154d9e1 100644 >> --- a/gcc/json.h >> +++ b/gcc/json.h >> @@ -107,7 +107,7 @@ class array : public value >> enum kind get_kind () const FINAL OVERRIDE { return JSON_ARRAY; } >> void print (pretty_printer *pp) const FINAL OVERRIDE; >> >> - void append (value *v) { m_elements.safe_push (v); } >> + void append (value *v); >> >> private: >> auto_vec<value *> m_elements; >> @@ -134,7 +134,7 @@ class number : public value >> class string : public value >> { >> public: >> - string (const char *utf8) : m_utf8 (xstrdup (utf8)) {} >> + string (const char *utf8); >> ~string () { free (m_utf8); } >> >> enum kind get_kind () const FINAL OVERRIDE { return JSON_STRING; } >> diff --git a/gcc/optinfo-emit-json.cc b/gcc/optinfo-emit-json.cc >> index bf1172a..6460a81 100644 >> --- a/gcc/optinfo-emit-json.cc >> +++ b/gcc/optinfo-emit-json.cc >> @@ -202,10 +202,12 @@ optrecord_json_writer::impl_location_to_json (dump_impl_location_t loc) >> json::object * >> optrecord_json_writer::location_to_json (location_t loc) >> { >> + gcc_assert (LOCATION_LOCUS (loc) != UNKNOWN_LOCATION); >> + expanded_location exploc = expand_location (loc); >> json::object *obj = new json::object (); >> - obj->set ("file", new json::string (LOCATION_FILE (loc))); >> - obj->set ("line", new json::number (LOCATION_LINE (loc))); >> - obj->set ("column", new json::number (LOCATION_COLUMN (loc))); >> + obj->set ("file", new json::string (exploc.file)); >> + obj->set ("line", new json::number (exploc.line)); >> + obj->set ("column", new json::number (exploc.column)); >> return obj; >> } >> >> @@ -330,7 +332,7 @@ optrecord_json_writer::inlining_chain_to_json (location_t loc) >> const char *printable_name >> = lang_hooks.decl_printable_name (fndecl, 2); >> obj->set ("fndecl", new json::string (printable_name)); >> - if (*locus != UNKNOWN_LOCATION) >> + if (LOCATION_LOCUS (*locus) != UNKNOWN_LOCATION) >> obj->set ("site", location_to_json (*locus)); >> array->append (obj); >> } >> @@ -371,8 +373,9 @@ optrecord_json_writer::optinfo_to_json (const optinfo *optinfo) >> json_item->set ("expr", new json::string (item->get_text ())); >> >> /* Capture any location for the node. */ >> - if (item->get_location () != UNKNOWN_LOCATION) >> - json_item->set ("location", location_to_json (item->get_location ())); >> + if (LOCATION_LOCUS (item->get_location ()) != UNKNOWN_LOCATION) >> + json_item->set ("location", >> + location_to_json (item->get_location ())); >> >> message->append (json_item); >> } >> @@ -383,8 +386,9 @@ optrecord_json_writer::optinfo_to_json (const optinfo *optinfo) >> json_item->set ("stmt", new json::string (item->get_text ())); >> >> /* Capture any location for the stmt. */ >> - if (item->get_location () != UNKNOWN_LOCATION) >> - json_item->set ("location", location_to_json (item->get_location ())); >> + if (LOCATION_LOCUS (item->get_location ()) != UNKNOWN_LOCATION) >> + json_item->set ("location", >> + location_to_json (item->get_location ())); >> >> message->append (json_item); >> } >> @@ -395,8 +399,9 @@ optrecord_json_writer::optinfo_to_json (const optinfo *optinfo) >> json_item->set ("symtab_node", new json::string (item->get_text ())); >> >> /* Capture any location for the node. */ >> - if (item->get_location () != UNKNOWN_LOCATION) >> - json_item->set ("location", location_to_json (item->get_location ())); >> + if (LOCATION_LOCUS (item->get_location ()) != UNKNOWN_LOCATION) >> + json_item->set ("location", >> + location_to_json (item->get_location ())); >> message->append (json_item); >> } >> break; >> diff --git a/gcc/testsuite/gcc.c-torture/compile/pr86636.c b/gcc/testsuite/gcc.c-torture/compile/pr86636.c >> new file mode 100644 >> index 0000000..2fe2f70 >> --- /dev/null >> +++ b/gcc/testsuite/gcc.c-torture/compile/pr86636.c >> @@ -0,0 +1,8 @@ >> +/* { dg-options "-fsave-optimization-record -ftree-loop-vectorize -ftree-parallelize-loops=2" } */ >> + >> +void >> +n2 (int ih) >> +{ >> + while (ih < 1) >> + ++ih; >> +} >> -- >> 1.8.5.3 >> Hi David, I believe the test in this patch needs a "{ dg-require-effective-target pthread }" as -ftree-parallelize-loops seems to be turning on -pthread. Cheers, Andre
diff --git a/gcc/json.cc b/gcc/json.cc index 3c2aa77..3ead980 100644 --- a/gcc/json.cc +++ b/gcc/json.cc @@ -76,12 +76,15 @@ object::print (pretty_printer *pp) const pp_character (pp, '}'); } -/* Set the json::value * for KEY, taking ownership of VALUE +/* Set the json::value * for KEY, taking ownership of V (and taking a copy of KEY if necessary). */ void object::set (const char *key, value *v) { + gcc_assert (key); + gcc_assert (v); + value **ptr = m_map.get (key); if (ptr) { @@ -126,6 +129,15 @@ array::print (pretty_printer *pp) const pp_character (pp, ']'); } +/* Append non-NULL value V to a json::array, taking ownership of V. */ + +void +array::append (value *v) +{ + gcc_assert (v); + m_elements.safe_push (v); +} + /* class json::number, a subclass of json::value, wrapping a double. */ /* Implementation of json::value::print for json::number. */ @@ -140,6 +152,16 @@ number::print (pretty_printer *pp) const /* class json::string, a subclass of json::value. */ +/* json::string's ctor. */ + +string::string (const char *utf8) +{ + gcc_assert (utf8); + m_utf8 = xstrdup (utf8); +} + +/* Implementation of json::value::print for json::string. */ + void string::print (pretty_printer *pp) const { diff --git a/gcc/json.h b/gcc/json.h index 5c3274c..154d9e1 100644 --- a/gcc/json.h +++ b/gcc/json.h @@ -107,7 +107,7 @@ class array : public value enum kind get_kind () const FINAL OVERRIDE { return JSON_ARRAY; } void print (pretty_printer *pp) const FINAL OVERRIDE; - void append (value *v) { m_elements.safe_push (v); } + void append (value *v); private: auto_vec<value *> m_elements; @@ -134,7 +134,7 @@ class number : public value class string : public value { public: - string (const char *utf8) : m_utf8 (xstrdup (utf8)) {} + string (const char *utf8); ~string () { free (m_utf8); } enum kind get_kind () const FINAL OVERRIDE { return JSON_STRING; } diff --git a/gcc/optinfo-emit-json.cc b/gcc/optinfo-emit-json.cc index bf1172a..6460a81 100644 --- a/gcc/optinfo-emit-json.cc +++ b/gcc/optinfo-emit-json.cc @@ -202,10 +202,12 @@ optrecord_json_writer::impl_location_to_json (dump_impl_location_t loc) json::object * optrecord_json_writer::location_to_json (location_t loc) { + gcc_assert (LOCATION_LOCUS (loc) != UNKNOWN_LOCATION); + expanded_location exploc = expand_location (loc); json::object *obj = new json::object (); - obj->set ("file", new json::string (LOCATION_FILE (loc))); - obj->set ("line", new json::number (LOCATION_LINE (loc))); - obj->set ("column", new json::number (LOCATION_COLUMN (loc))); + obj->set ("file", new json::string (exploc.file)); + obj->set ("line", new json::number (exploc.line)); + obj->set ("column", new json::number (exploc.column)); return obj; } @@ -330,7 +332,7 @@ optrecord_json_writer::inlining_chain_to_json (location_t loc) const char *printable_name = lang_hooks.decl_printable_name (fndecl, 2); obj->set ("fndecl", new json::string (printable_name)); - if (*locus != UNKNOWN_LOCATION) + if (LOCATION_LOCUS (*locus) != UNKNOWN_LOCATION) obj->set ("site", location_to_json (*locus)); array->append (obj); } @@ -371,8 +373,9 @@ optrecord_json_writer::optinfo_to_json (const optinfo *optinfo) json_item->set ("expr", new json::string (item->get_text ())); /* Capture any location for the node. */ - if (item->get_location () != UNKNOWN_LOCATION) - json_item->set ("location", location_to_json (item->get_location ())); + if (LOCATION_LOCUS (item->get_location ()) != UNKNOWN_LOCATION) + json_item->set ("location", + location_to_json (item->get_location ())); message->append (json_item); } @@ -383,8 +386,9 @@ optrecord_json_writer::optinfo_to_json (const optinfo *optinfo) json_item->set ("stmt", new json::string (item->get_text ())); /* Capture any location for the stmt. */ - if (item->get_location () != UNKNOWN_LOCATION) - json_item->set ("location", location_to_json (item->get_location ())); + if (LOCATION_LOCUS (item->get_location ()) != UNKNOWN_LOCATION) + json_item->set ("location", + location_to_json (item->get_location ())); message->append (json_item); } @@ -395,8 +399,9 @@ optrecord_json_writer::optinfo_to_json (const optinfo *optinfo) json_item->set ("symtab_node", new json::string (item->get_text ())); /* Capture any location for the node. */ - if (item->get_location () != UNKNOWN_LOCATION) - json_item->set ("location", location_to_json (item->get_location ())); + if (LOCATION_LOCUS (item->get_location ()) != UNKNOWN_LOCATION) + json_item->set ("location", + location_to_json (item->get_location ())); message->append (json_item); } break; diff --git a/gcc/testsuite/gcc.c-torture/compile/pr86636.c b/gcc/testsuite/gcc.c-torture/compile/pr86636.c new file mode 100644 index 0000000..2fe2f70 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/pr86636.c @@ -0,0 +1,8 @@ +/* { dg-options "-fsave-optimization-record -ftree-loop-vectorize -ftree-parallelize-loops=2" } */ + +void +n2 (int ih) +{ + while (ih < 1) + ++ih; +}