Message ID | 1420827419-18655-3-git-send-email-rth@twiddle.net |
---|---|
State | New, archived |
Headers |
Received: (qmail 4666 invoked by alias); 9 Jan 2015 18:17:41 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 4542 invoked by uid 89); 9 Jan 2015 18:17:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL, BAYES_00, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-qg0-f42.google.com X-Received: by 10.140.108.74 with SMTP id i68mr27742572qgf.35.1420827455869; Fri, 09 Jan 2015 10:17:35 -0800 (PST) From: Richard Henderson <rth@twiddle.net> To: libc-alpha@sourceware.org Cc: roland@hack.frob.com Subject: [PATCH B2/2] Use builtin_unreachable in assert Date: Fri, 9 Jan 2015 10:16:59 -0800 Message-Id: <1420827419-18655-3-git-send-email-rth@twiddle.net> In-Reply-To: <1420827419-18655-1-git-send-email-rth@twiddle.net> References: <1420827419-18655-1-git-send-email-rth@twiddle.net> |
Commit Message
Richard Henderson
Jan. 9, 2015, 6:16 p.m. UTC
Finally, use __builtin_unreachable to tell the compiler about impossible paths. The amount of code change with gcc 4.9.2 is enormous, and is usually difficult to tell whether the change is definitely for the better. Most of the time the difference begins with a branch test being reversed, after which none of the remainder of the code lines up. I did see one definitely positive case, gconv_builtin.os, which is small enough to evaluate easily: - 20: 48 8b 75 00 mov 0x0(%rbp),%rsi - 24: 4c 89 f7 mov %r14,%rdi - 27: e8 00 00 00 00 callq 2c <__gconv_get_builtin_trans+0x2c> - 28: R_X86_64_PLT32 __GI_strcmp-0x4 - 2c: 85 c0 test %eax,%eax - 2e: 74 0e je 3e <__gconv_get_builtin_trans+0x3e> - 30: 48 83 c3 01 add $0x1,%rbx - 34: 48 83 c5 20 add $0x20,%rbp - 38: 48 83 fb 0c cmp $0xc,%rbx - 3c: 75 e2 jne 20 <__gconv_get_builtin_trans+0x20> + 20: 48 83 c5 01 add $0x1,%rbp + 24: 48 83 c3 20 add $0x20,%rbx + 28: 48 8b 33 mov (%rbx),%rsi + 2b: 4c 89 f7 mov %r14,%rdi + 2e: e8 00 00 00 00 callq 33 <__gconv_get_builtin_trans+0x33> + 2f: R_X86_64_PLT32 __GI_strcmp-0x4 + 33: 85 c0 test %eax,%eax + 35: 75 e9 jne 20 <__gconv_get_builtin_trans+0x20> The assert following the loop results in the elimination of the loop comparison, leaving only the strcmp test controlling loop exit. In leiu of investigaing all such, here's the final size comparison: text data bss dec hex filename 1623772 20968 18160 1662900 195fb4 bld-A1/libc.so 1623788 20968 18160 1662916 195fc4 bld-B1/libc.so 1623604 20968 18160 1662732 195f0c bld-B2/libc.so r~ PS: Note that __builtin_unreachable was introduced in gcc 4.5, therefore no version test is required. * include/assert.h (assert): Use __builtin_unreachable. (assert_perror): Use __builtin_unreachable. --- include/assert.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Comments
On 09 Jan 2015 10:16, Richard Henderson wrote: > Finally, use __builtin_unreachable to tell the compiler about > impossible paths. i'm not sure this is the way to go. the expectation (as required by POSIX) is that you're guaranteed there are no side-effects when NDEBUG is applied to assert. by using __builtin_unreachable, that is no longer the case. yes, assert in this context is internal to glibc and so doesn't have to conform to POSIX directly. but i think forcing developers to remember this kind of nuance just leads to bugs, especially when you consider glibc imports code from other projects (like gnulib). the code shrinkage is attractive, but the other factors considered (and that NDEBUG usage in glibc in general is uncommon) leads me to think this path is not worth the pain. -mike
On 03/01/2015 11:43 PM, Mike Frysinger wrote: > On 09 Jan 2015 10:16, Richard Henderson wrote: >> Finally, use __builtin_unreachable to tell the compiler about >> impossible paths. > > i'm not sure this is the way to go. the expectation (as required by POSIX) > is that you're guaranteed there are no side-effects when NDEBUG is applied > to assert. by using __builtin_unreachable, that is no longer the case. I wonder if, during the gcc 6 development cycle, we should experiment with a __builtin_side_effects_p, akin to __builtin_constant_p. Then we could still follow POSIX re no side effects but even in the external context provide the information derivable from __builtin_unreachable. I.e. #define assert(X) \ (__builtin_side_effects_p (X) || !(X) ? 0 : __builtin_unreachable ()) r~
On Mon, Mar 02, 2015 at 09:18:31AM -0800, Richard Henderson wrote: > On 03/01/2015 11:43 PM, Mike Frysinger wrote: > > On 09 Jan 2015 10:16, Richard Henderson wrote: > >> Finally, use __builtin_unreachable to tell the compiler about > >> impossible paths. > > > > i'm not sure this is the way to go. the expectation (as required by POSIX) > > is that you're guaranteed there are no side-effects when NDEBUG is applied > > to assert. by using __builtin_unreachable, that is no longer the case. > > I wonder if, during the gcc 6 development cycle, we should experiment with a > __builtin_side_effects_p, akin to __builtin_constant_p. Then we could still > follow POSIX re no side effects but even in the external context provide the > information derivable from __builtin_unreachable. I.e. > > #define assert(X) \ > (__builtin_side_effects_p (X) || !(X) ? 0 : __builtin_unreachable ()) Tom wrote a patch implementing something like that a while back (for C only) <https://gcc.gnu.org/ml/gcc-patches/2014-04/msg01984.html>, but the patch never made it in. Maybe I could revive and adjust it during gcc 6 stage 1 if it would help you. Marek
> i'm not sure this is the way to go. the expectation (as required by POSIX) > is that you're guaranteed there are no side-effects when NDEBUG is applied > to assert. by using __builtin_unreachable, that is no longer the case. The requirement (which should be in ISO C, not in POSIX) is specified at the C language level. "No side effects" at the C level certainly does not preclude a different correct translation of the program to machine code. > the code shrinkage is attractive, but the other factors considered (and that > NDEBUG usage in glibc in general is uncommon) leads me to think this path is > not worth the pain. In fact, many distributions build libc with -DNDEBUG. In terms of quantity of execution that happens, I'd bet most use of libc is with -DNDEBUG. Thanks, Roland
> I wonder if, during the gcc 6 development cycle, we should experiment with a > __builtin_side_effects_p, akin to __builtin_constant_p. Then we could still > follow POSIX re no side effects but even in the external context provide the > information derivable from __builtin_unreachable. I.e. > > #define assert(X) \ > (__builtin_side_effects_p (X) || !(X) ? 0 : __builtin_unreachable ()) It would be ideal to do this in a way that some new warning option or fortify enablement or whatnot can give compile-time warnings about side effects in assert expressions. How uselessly false-positivey that would be depends on how much a particular codebase uses the style of data gathering or consistency-checking function calls in assert expressions (i.e., things that the compiler would consider to have side effects, perhaps even with complete and perfect inlining, but the programmer reasonably considers to have no actual semantic side effects).
On 02 Mar 2015 14:56, Roland McGrath wrote: > > i'm not sure this is the way to go. the expectation (as required by POSIX) > > is that you're guaranteed there are no side-effects when NDEBUG is applied > > to assert. by using __builtin_unreachable, that is no longer the case. > > The requirement (which should be in ISO C, not in POSIX) is specified at > the C language level. i don't have an ISO C reference, so i look at POSIX, and it explicitly defines it this way. > "No side effects" at the C level certainly does not > preclude a different correct translation of the program to machine code. sure ... i wasn't saying otherwise. my point is that __builtin_unreachable doesn't do that -- it explicitly is documented by gcc as: If control flow reaches the point of the __builtin_unreachable, the program is undefined. if there was a solution as Richard alluded to that allows for expansion of the expression all the time w/out breaking things when the assert is actually hit, i'm all for that. -mike
> > "No side effects" at the C level certainly does not > > preclude a different correct translation of the program to machine code. > > sure ... i wasn't saying otherwise. my point is that __builtin_unreachable > doesn't do that -- it explicitly is documented by gcc as: > If control flow reaches the point of the __builtin_unreachable, the program is > undefined. > > if there was a solution as Richard alluded to that allows for expansion of the > expression all the time w/out breaking things when the assert is actually hit, > i'm all for that. I don't understand what your concern is. A call to __assert_fail leads to a call to abort and that is no more or less well-defined or recoverable than __builtin_unreachable.
On 04 Mar 2015 11:25, Roland McGrath wrote: > > > "No side effects" at the C level certainly does not > > > preclude a different correct translation of the program to machine code. > > > > sure ... i wasn't saying otherwise. my point is that __builtin_unreachable > > doesn't do that -- it explicitly is documented by gcc as: > > If control flow reaches the point of the __builtin_unreachable, the program is > > undefined. > > > > if there was a solution as Richard alluded to that allows for expansion of the > > expression all the time w/out breaking things when the assert is actually hit, > > i'm all for that. > > I don't understand what your concern is. A call to __assert_fail leads to > a call to abort and that is no more or less well-defined or recoverable > than __builtin_unreachable. yes, when NDEBUG is not defined. but that's not what we're discussing here. if your intention is for assert statements to crash the program even when NDEBUG is defined, then what exactly is the point of it ? why don't we just delete it instead ? -mike
diff --git a/include/assert.h b/include/assert.h index d7e2759..bd3ef51 100644 --- a/include/assert.h +++ b/include/assert.h @@ -29,6 +29,6 @@ hidden_proto (__assert_perror_fail) #ifdef NDEBUG # undef assert # undef assert_perror -# define assert(expr) ((void)(0 && (expr))) -# define assert_perror(errnum) ((void)(0 && (errnum))) +# define assert(e) ((e) ? (void)0 : __builtin_unreachable ()) +# define assert_perror(e) (!(e) ? (void)0 : __builtin_unreachable ()) #endif