From patchwork Wed Aug 10 10:12:26 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 657659 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3s8RmD42rYz9t0F for ; Wed, 10 Aug 2016 20:12:43 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=KrYWSJKQ; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=M5KlGCUZhRzqag4X2 RJfmIkJBz/rwBdGrldKxDDOZnwYVfoLH+3neDXWYLHdbzvqMGyZLbNZQ3vneQziy ik+YzDW1zJ+frWTHzDxUVXQl8UYs3V7WFGJ5DGrJ+s40fwD9FCo7J2z7WubPeoW+ /bNXstAaszHcka0aje7CGRQfso= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=RRLTPOSrgkAfcdLRLw0eQwB 8gQo=; b=KrYWSJKQL0zJomoJkiGtoTfzjhV2I9CziSNwFXw3YJncy+8Ho3oXZ9P H4CAPTUH1jJ6tc5DlTwKVvnwYGJMWFQVcMlfI+8MsTMzvw6OE+upgcHZ8cQhYqnZ zh8jrmWp4Ku62U38D8/Wyo0pSQZbeNaHFGEK0AwW3G9qiKImGoO8= Received: (qmail 2652 invoked by alias); 10 Aug 2016 10:12:34 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 2642 invoked by uid 89); 10 Aug 2016 10:12:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=GCC_VERSION, gcc_version, snprintf, *tmp X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 10 Aug 2016 10:12:32 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A0C7283F45; Wed, 10 Aug 2016 10:12:30 +0000 (UTC) Received: from reynosa.quesejoda.com (vpn1-4-203.ams2.redhat.com [10.36.4.203]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u7AACR2T000461; Wed, 10 Aug 2016 06:12:27 -0400 Subject: Re: protected alloca class for malloc fallback To: Richard Biener References: <57A32741.7010003@redhat.com> <57A3F57F.3050509@gmail.com> <57A4A5E8.90205@redhat.com> <57A9D7E5.6090208@redhat.com> Cc: Martin Sebor , gcc-patches , Pedro Alves From: Aldy Hernandez Message-ID: <57AAFE0A.4050306@redhat.com> Date: Wed, 10 Aug 2016 06:12:26 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: On 08/10/2016 06:04 AM, Richard Biener wrote: > On Tue, Aug 9, 2016 at 3:17 PM, Aldy Hernandez 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 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 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 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 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 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 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 chunk; + chunk.reserve (length); + buffer = chunk.address (); length = 0; for (c = c1; c <= c2; ++c)