protected alloca class for malloc fallback
diff mbox

Message ID 57AAFE0A.4050306@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Aug. 10, 2016, 10:12 a.m. UTC
On 08/10/2016 06:04 AM, Richard Biener wrote:
> On Tue, Aug 9, 2016 at 3:17 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
>> On 08/05/2016 01:55 PM, Richard Biener wrote:
>>
>> Hi Richard.
>>
>>> Please don't use std::string.  For string building you can use obstacks.
>>
>>
>> Alright let's talk details then so I can write things up in a way you
>> approve of.
>>
>> Take for instance simple uses like all the tree_*check_failed routines,
>> which I thought were great candidates for std::string-- they're going to be
>> outputted to the screen or disk which is clearly many times more expensive
>> than the malloc or overhead of std::string:
>>
>>        length += strlen ("expected ");
>>        buffer = tmp = (char *) alloca (length);
>>        length = 0;
>>        while ((code = (enum tree_code) va_arg (args, int)))
>>          {
>>            const char *prefix = length ? " or " : "expected ";
>>
>>            strcpy (tmp + length, prefix);
>>            length += strlen (prefix);
>>            strcpy (tmp + length, get_tree_code_name (code));
>>            length += strlen (get_tree_code_name (code));
>>          }
>>
>> Do you suggest using obstacks here, or did you have something else in mind?
>
> Why would you want to get rid of the alloca here?
>
>> How about if it's something like the above but there are multiple exit
>> points from the function.  I mean, we still need to call obstack_free() on
>> all exit points, which is what I wanted to avoid for something as simple as
>> building a throw-away string.
>
> But you'll end up in
>
>    internal_error ("tree check: %s, have %s in %s, at %s:%d",
>                    buffer, get_tree_code_name (TREE_CODE (node)),
>                    function, trim_filename (file), line);
>
> where you have an interface using C strings again.
>
> It's not that the standard library is bad per-se, it's just using a
> tool that doesn't
> fit its uses.  This makes the code a messy mix of two concepts which is what
> I object to.
>
> Yes, the above example may have premature optimization applied.  Is that
> a reason to re-write it using std::string?  No.  Is it a reason to rewrite it
> using obstracks?  No.  Just leave it alone.

How about we use auto_vec<> with an expected sane default?  This way we 
have a performance conscious solution that will fallback to malloc if 
necessary, while cleaning up after itself without the need for changing 
every exit point from a function.

For example, the attached untested patch implements this approach for 
tree.c.

Would this be acceptable?

Aldy

Comments

Richard Biener Aug. 10, 2016, 10:39 a.m. UTC | #1
On Wed, Aug 10, 2016 at 12:12 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> On 08/10/2016 06:04 AM, Richard Biener wrote:
>>
>> On Tue, Aug 9, 2016 at 3:17 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
>>>
>>> On 08/05/2016 01:55 PM, Richard Biener wrote:
>>>
>>> Hi Richard.
>>>
>>>> Please don't use std::string.  For string building you can use obstacks.
>>>
>>>
>>>
>>> Alright let's talk details then so I can write things up in a way you
>>> approve of.
>>>
>>> Take for instance simple uses like all the tree_*check_failed routines,
>>> which I thought were great candidates for std::string-- they're going to
>>> be
>>> outputted to the screen or disk which is clearly many times more
>>> expensive
>>> than the malloc or overhead of std::string:
>>>
>>>        length += strlen ("expected ");
>>>        buffer = tmp = (char *) alloca (length);
>>>        length = 0;
>>>        while ((code = (enum tree_code) va_arg (args, int)))
>>>          {
>>>            const char *prefix = length ? " or " : "expected ";
>>>
>>>            strcpy (tmp + length, prefix);
>>>            length += strlen (prefix);
>>>            strcpy (tmp + length, get_tree_code_name (code));
>>>            length += strlen (get_tree_code_name (code));
>>>          }
>>>
>>> Do you suggest using obstacks here, or did you have something else in
>>> mind?
>>
>>
>> Why would you want to get rid of the alloca here?
>>
>>> How about if it's something like the above but there are multiple exit
>>> points from the function.  I mean, we still need to call obstack_free()
>>> on
>>> all exit points, which is what I wanted to avoid for something as simple
>>> as
>>> building a throw-away string.
>>
>>
>> But you'll end up in
>>
>>    internal_error ("tree check: %s, have %s in %s, at %s:%d",
>>                    buffer, get_tree_code_name (TREE_CODE (node)),
>>                    function, trim_filename (file), line);
>>
>> where you have an interface using C strings again.
>>
>> It's not that the standard library is bad per-se, it's just using a
>> tool that doesn't
>> fit its uses.  This makes the code a messy mix of two concepts which is
>> what
>> I object to.
>>
>> Yes, the above example may have premature optimization applied.  Is that
>> a reason to re-write it using std::string?  No.  Is it a reason to rewrite
>> it
>> using obstracks?  No.  Just leave it alone.
>
>
> How about we use auto_vec<> with an expected sane default?  This way we have
> a performance conscious solution that will fallback to malloc if necessary,
> while cleaning up after itself without the need for changing every exit
> point from a function.
>
> For example, the attached untested patch implements this approach for
> tree.c.
>
> Would this be acceptable?

Again - why change way from alloca in the tree_*_check_fail routines?

get_file_function_name may be seen as working on "arbitrary" user input
(filenames), but I'm not sure I'd treat it that way.  The function computes
strings that are the same over the whole compilation (but it does so
possibly repeatedly), so simply using malloc and having the result cached
would work here, too.

Richard.

> Aldy
>

Patch
diff mbox

diff --git a/gcc/tree.c b/gcc/tree.c
index 11d3b51..160c539 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -9636,9 +9636,9 @@  anon_aggrname_format()
 tree
 get_file_function_name (const char *type)
 {
-  char *buf;
   const char *p;
   char *q;
+  auto_vec<char, 100> chunk;
 
   /* If we already have a name we know to be unique, just use that.  */
   if (first_global_object_name)
@@ -9674,7 +9674,8 @@  get_file_function_name (const char *type)
 	file = LOCATION_FILE (input_location);
 
       len = strlen (file);
-      q = (char *) alloca (9 + 17 + len + 1);
+      chunk.reserve (9 + 17 + len + 1);
+      q = chunk.address ();
       memcpy (q, file, len + 1);
 
       snprintf (q + len, 9 + 17 + 1, "_%08X_" HOST_WIDE_INT_PRINT_HEX, 
@@ -9684,16 +9685,16 @@  get_file_function_name (const char *type)
     }
 
   clean_symbol_name (q);
-  buf = (char *) alloca (sizeof (FILE_FUNCTION_FORMAT) + strlen (p)
-			 + strlen (type));
 
   /* Set up the name of the file-level functions we may need.
      Use a global object (which is already required to be unique over
      the program) rather than the file name (which imposes extra
      constraints).  */
-  sprintf (buf, FILE_FUNCTION_FORMAT, type, p);
+  auto_vec<char, 100> buf;
+  buf.reserve (sizeof (FILE_FUNCTION_FORMAT) + strlen (p) + strlen (type));
+  sprintf (buf.address (), FILE_FUNCTION_FORMAT, type, p);
 
-  return get_identifier (buf);
+  return get_identifier (buf.address ());
 }
 
 #if defined ENABLE_TREE_CHECKING && (GCC_VERSION >= 2007)
@@ -9711,6 +9712,7 @@  tree_check_failed (const_tree node, const char *file,
   const char *buffer;
   unsigned length = 0;
   enum tree_code code;
+  auto_vec<char, 150> chunk;
 
   va_start (args, function);
   while ((code = (enum tree_code) va_arg (args, int)))
@@ -9721,7 +9723,8 @@  tree_check_failed (const_tree node, const char *file,
       char *tmp;
       va_start (args, function);
       length += strlen ("expected ");
-      buffer = tmp = (char *) alloca (length);
+      chunk.reserve (length);
+      buffer = tmp = chunk.address ();
       length = 0;
       while ((code = (enum tree_code) va_arg (args, int)))
 	{
@@ -9760,7 +9763,9 @@  tree_not_check_failed (const_tree node, const char *file,
     length += 4 + strlen (get_tree_code_name (code));
   va_end (args);
   va_start (args, function);
-  buffer = (char *) alloca (length);
+  auto_vec<char, 150> chunk;
+  chunk.reserve (length);
+  buffer = chunk.address ();
   length = 0;
   while ((code = (enum tree_code) va_arg (args, int)))
     {
@@ -9809,7 +9814,9 @@  tree_range_check_failed (const_tree node, const char *file, int line,
     length += 4 + strlen (get_tree_code_name ((enum tree_code) c));
 
   length += strlen ("expected ");
-  buffer = (char *) alloca (length);
+  auto_vec<char, 150> chunk;
+  chunk.reserve (length);
+  buffer = chunk.address ();
   length = 0;
 
   for (c = c1; c <= c2; ++c)
@@ -9870,7 +9877,9 @@  omp_clause_range_check_failed (const_tree node, const char *file, int line,
     length += 4 + strlen (omp_clause_code_name[c]);
 
   length += strlen ("expected ");
-  buffer = (char *) alloca (length);
+  auto_vec<char, 150> chunk;
+  chunk.reserve (length);
+  buffer = chunk.address ();
   length = 0;
 
   for (c = c1; c <= c2; ++c)