This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix segfault in -fsave-optimization-record (PR tree-optimization/86636)


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]