From patchwork Thu Aug 4 11:30:09 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 655762 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 3s4nmp2Wz0z9s2G for ; Thu, 4 Aug 2016 21:30:32 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=fgZiuMhc; 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:from :subject:to:message-id:date:mime-version:content-type; q=dns; s= default; b=bNd2sLpaC7DWHEbMtrGeJHsgtKQR25rqi9DkyZ6iP2wltWNGE3plO byr1L7aWvFPdxv/OrnTM6qNdh2HOa1gpm1x4jDSGFJlQDB+eA+9uYR1dDgySuf1o 9UiJEosBcWOysXTxgVPfiCUF6XFfjglUmbaxSruo15t+6Ms6X9SVBY= 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:from :subject:to:message-id:date:mime-version:content-type; s= default; bh=BcaXzIbNnZ49N9PU2YfW7o0SK+g=; b=fgZiuMhci6yagMg8fIRf mULHkWBuLPPg2axevN13vIgdOHCm4mwIOo/LGV2wqPIWTBBR/TyPeWvPAhWQ0E5j 881HLS9vw1QGZFy5bwckb+/WTYiTW/7TzuuOp5uqooHY9Kix8/iBoHbQGBqw3FJw S4hytS8VwhoO0C6+VfogArQ= Received: (qmail 73241 invoked by alias); 4 Aug 2016 11:30:26 -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 73216 invoked by uid 89); 4 Aug 2016 11:30:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=blessed, 50000, fear, *str 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; Thu, 04 Aug 2016 11:30:12 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (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 299FF4E333 for ; Thu, 4 Aug 2016 11:30:11 +0000 (UTC) Received: from reynosa.quesejoda.com (vpn1-7-213.ams2.redhat.com [10.36.7.213]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u74BU9D6001831 for ; Thu, 4 Aug 2016 07:30:10 -0400 From: Aldy Hernandez Subject: protected alloca class for malloc fallback To: gcc-patches Message-ID: <57A32741.7010003@redhat.com> Date: Thu, 4 Aug 2016 07:30:09 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 Howdy! As part of my -Walloca-larger-than=life work, I've been running said pass over gcc, binutils, and gdb, and trying to fix things along the way. Particularly irritating and error prone is having to free malloc'd pointers on every function exit point. We end up with a lot of: foo(size_t len) { void *p, *m_p = NULL; if (len < HUGE) p = alloca(len); else p = m_p = malloc(len); if (something) goto out; stuff(); out: free (m_p); } ...which nobody really likes. I've been thinking that for GCC we could have a protected_alloca class whose destructor frees any malloc'd memory: void foo() { char *p; protected_alloca chunk(50000); p = (char *) chunk.pointer(); f(p); } This would generate: void foo() () { void * _3; : _3 = malloc (50000); f (_3); : free (_3); [tail call] return; } Now the problem with this is that the memory allocated by chunk is freed when it goes out of scope, which may not be what you want. For example: func() { char *str; { protected_alloca chunk (99999999); // malloc'd pointer will be freed when chunk goes out of scope. str = (char *) chunk.pointer (); } use (str); // BAD! Use after free. } In the attached patch implementing this class I have provided another idiom for avoiding this problem: func() { void *ptr; protected_alloca chunk; { chunk.alloc (9999999); str = (char *) chunk.pointer (); } // OK, pointer will be freed on function exit. use (str); } So I guess it's between annoying gotos and keeping track of multiple exit points to a function previously calling alloca, or making sure the protected_alloca object always resides in the scope where the memory is going to be used. Is there a better blessed C++ way? If not, is this OK? Included is the conversion of tree.c. More to follow once we agree on a solution. Tested on x86-64 Linux. Aldy commit fd0078ef60dd75ab488392e0e05b28f27d971bdf Author: Aldy Hernandez Date: Thu Aug 4 06:43:37 2016 -0400 * protected-alloca.h: New. * tree.c (get_file_function_name): Use protected_alloca. (tree_check_failed): Same. (tree_not_check_failed): Same. (tree_range_check_failed): Same. (omp_clause_range_check_failed): Same. diff --git a/gcc/protected-alloca.h b/gcc/protected-alloca.h new file mode 100644 index 0000000..62f2a7b --- /dev/null +++ b/gcc/protected-alloca.h @@ -0,0 +1,113 @@ +/* Alloca wrapper with a malloc fallback. + Copyright (C) 2016 Free Software Foundation, Inc. + Contributed by Aldy Hernandez . + +This file is part of GCC. + +GCC is free software; you can redistribute it and/or modify +it under the terms of the GNU General Public License as published by +the Free Software Foundation; either version 3, or (at your option) +any later version. + +GCC is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU General Public License for more details. + +You should have received a copy of the GNU General Public License +along with GCC; see the file COPYING3. If not see +. */ + +#ifndef _PROTECTED_ALLOCA_H_ +#define _PROTECTED_ALLOCA_H_ + +#ifndef MAX_ALLOCA_SIZE +#define MAX_ALLOCA_SIZE 4096 +#endif + +#ifdef __GNUC__ +#define _ALLOCA_INLINE_ __attribute__((always_inline)) +#else +#define _ALLOCA_INLINE_ +#endif + +/* This is a wrapper class for alloca that falls back to malloc if the + allocation size is > MAX_ALLOCA_SIZE. It is meant to replace: + + char *str = (char *) alloca (N); + + by this: + + protected_alloca chunk (N); + char *str = (char *) chunk.pointer (); + + or this: + + protected_alloca chunk; + chunk.alloc (N); + char *str = (char *) chunk.pointer (); + + If N > MAX_ALLOCA_SIZE, malloc is used, and whenever `chunk' goes + out of scope, the malloc'd memory will be freed. Keep in mind that + the malloc'd memory gets freed when `chunk' goes out of scope, and + may be freed earlier than expected. For example: + + func() + { + char *str; + { + protected_alloca chunk (99999999); + // If malloc'd, pointer will be freed when chunk goes out of scope. + str = (char *) chunk.pointer (); + } + use (str); // BAD! Use after free. + } + + In this case, it is best to use the following idiom: + + func() + { + void *ptr; + protected_alloca chunk; + { + chunk.alloc (9999999); + str = (char *) chunk.pointer (); + } + // OK, pointer will be freed on function exit. + use (str); + } +*/ + +class protected_alloca { + void *p; + bool on_heap; +public: + // GCC will refuse to inline a function that calls alloca unless + // `always_inline' is used, for fear of inlining an alloca call + // appearing in a loop. Inlining this constructor won't make code + // any worse than it already is wrt loops. We know what we're + // doing. + _ALLOCA_INLINE_ void + internal_alloc (size_t size) + { + if (size < MAX_ALLOCA_SIZE) + { + p = alloca (size); + on_heap = false; + } + else + { + p = xmalloc (size); + on_heap = true; + } + } + _ALLOCA_INLINE_ + protected_alloca (size_t size) { internal_alloc (size); } + protected_alloca () { p = NULL; on_heap = false; } + ~protected_alloca () { if (on_heap) free (p); } + __attribute__((always_inline)) + void alloc (size_t size) { gcc_assert (!p); internal_alloc (size); } + void *pointer () { return p; } +}; + +#endif // _PROTECTED_ALLOCA_H_ diff --git a/gcc/tree.c b/gcc/tree.c index 11d3b51..621dcd5 100644 --- a/gcc/tree.c +++ b/gcc/tree.c @@ -62,6 +62,7 @@ along with GCC; see the file COPYING3. If not see #include "print-tree.h" #include "ipa-utils.h" #include "selftest.h" +#include "protected-alloca.h" /* Tree code classes. */ @@ -9639,6 +9640,7 @@ get_file_function_name (const char *type) char *buf; const char *p; char *q; + protected_alloca q_alloca; /* If we already have a name we know to be unique, just use that. */ if (first_global_object_name) @@ -9674,7 +9676,8 @@ get_file_function_name (const char *type) file = LOCATION_FILE (input_location); len = strlen (file); - q = (char *) alloca (9 + 17 + len + 1); + q_alloca.alloc (9 + 17 + len + 1); + q = (char *) q_alloca.pointer (); memcpy (q, file, len + 1); snprintf (q + len, 9 + 17 + 1, "_%08X_" HOST_WIDE_INT_PRINT_HEX, @@ -9684,8 +9687,9 @@ get_file_function_name (const char *type) } clean_symbol_name (q); - buf = (char *) alloca (sizeof (FILE_FUNCTION_FORMAT) + strlen (p) - + strlen (type)); + protected_alloca buf_alloca (sizeof (FILE_FUNCTION_FORMAT) + strlen (p) + + strlen (type)); + buf = (char *) buf_alloca.pointer (); /* Set up the name of the file-level functions we may need. Use a global object (which is already required to be unique over @@ -9709,7 +9713,8 @@ tree_check_failed (const_tree node, const char *file, { va_list args; const char *buffer; - unsigned length = 0; + protected_alloca buffer_alloca; + size_t length = 0; enum tree_code code; va_start (args, function); @@ -9721,7 +9726,8 @@ tree_check_failed (const_tree node, const char *file, char *tmp; va_start (args, function); length += strlen ("expected "); - buffer = tmp = (char *) alloca (length); + buffer_alloca.alloc (length); + buffer = tmp = (char *) buffer_alloca.pointer (); length = 0; while ((code = (enum tree_code) va_arg (args, int))) { @@ -9752,7 +9758,8 @@ tree_not_check_failed (const_tree node, const char *file, { va_list args; char *buffer; - unsigned length = 0; + protected_alloca buffer_alloca; + size_t length = 0; enum tree_code code; va_start (args, function); @@ -9760,7 +9767,8 @@ 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); + buffer_alloca.alloc (length); + buffer = (char *) buffer_alloca.pointer (); length = 0; while ((code = (enum tree_code) va_arg (args, int))) { @@ -9802,14 +9810,16 @@ tree_range_check_failed (const_tree node, const char *file, int line, enum tree_code c2) { char *buffer; - unsigned length = 0; + protected_alloca buffer_alloca; + size_t length = 0; unsigned int c; for (c = c1; c <= c2; ++c) length += 4 + strlen (get_tree_code_name ((enum tree_code) c)); length += strlen ("expected "); - buffer = (char *) alloca (length); + buffer_alloca.alloc (length); + buffer = (char *) buffer_alloca.pointer (); length = 0; for (c = c1; c <= c2; ++c) @@ -9863,14 +9873,16 @@ omp_clause_range_check_failed (const_tree node, const char *file, int line, enum omp_clause_code c2) { char *buffer; - unsigned length = 0; + protected_alloca buffer_alloca; + size_t length = 0; unsigned int c; for (c = c1; c <= c2; ++c) length += 4 + strlen (omp_clause_code_name[c]); length += strlen ("expected "); - buffer = (char *) alloca (length); + buffer_alloca.alloc (length); + buffer = (char *) buffer_alloca.pointer (); length = 0; for (c = c1; c <= c2; ++c)