From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from brightrain.aerifal.cx (brightrain.aerifal.cx [216.12.86.13]) by sourceware.org (Postfix) with ESMTPS id DCD523858D33 for ; Tue, 24 Jan 2023 11:23:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DCD523858D33 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=libc.org Authentication-Results: sourceware.org; spf=none smtp.mailfrom=libc.org Date: Tue, 24 Jan 2023 06:23:07 -0500 From: Rich Felker To: Florian Weimer Cc: Paul Pluzhnikov via Libc-alpha , Paul Pluzhnikov , Jonathan Wakely Subject: Re: [patch] Use __builtin_FILE and __builtin_LINE in assert implementation in C++ Message-ID: <20230124112307.GE3298@brightrain.aerifal.cx> References: <871qnlfhtv.fsf@oldenburg.str.redhat.com> <20230124111019.GC3298@brightrain.aerifal.cx> <878rhsgoxn.fsf@oldenburg.str.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <878rhsgoxn.fsf@oldenburg.str.redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Tue, Jan 24, 2023 at 12:19:32PM +0100, Florian Weimer wrote: > * Rich Felker: > > > On Mon, Jan 23, 2023 at 03:26:04PM +0100, Florian Weimer via Libc-alpha wrote: > >> * Paul Pluzhnikov via Libc-alpha: > >> > >> > diff --git a/assert/assert.h b/assert/assert.h > >> > index 72209bc5e7..4e0303db8d 100644 > >> > --- a/assert/assert.h > >> > +++ b/assert/assert.h > >> > @@ -89,7 +89,8 @@ __END_DECLS > >> > # define assert(expr) \ > >> > (static_cast (expr) > >> > \ > >> > ? void (0) \ > >> > - : __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION)) > >> > + : __assert_fail (#expr, __builtin_FILE (), __builtin_LINE (), \ > >> > + __ASSERT_FUNCTION)) > >> > # elif !defined __GNUC__ || defined __STRICT_ANSI__ > >> > # define assert(expr) \ > >> > ((expr) \ > >> > >> I think __builtin_FILE and __builtin_LINE are farily recent GCC/Clang > >> additions, so they need compiler version checks or a __has_builtin gate. > >> > >> Ideally, we would use here, but I don't think we can > >> include that from . > >> > >> Thanks, > >> Florian > > > > Why not instead: > > > > +static char *__file_for_assert = __FILE__; > > ... > > - : __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION)) > > + : __assert_fail (#expr, __file_for_assert, __LINE__, __ASSERT_FUNCTION)) > > > > ?? > > __FILE__ expansion needs to be delayed, otherwise it refers to assert.h. > > I think the builtins also have the advantage that they avoid ODR > violations because the definition is the same no matter what the file is > called. This issue is not restricted to modules. But maybe the right > answer is to blame the programmer and just add a diagnostic? > > Jonathan, I think the libstdc++ headers have the same issue. Thoughts? Arguably use of assert() in inline functions subject to this rule is a programming error, since there's no guarantee it expands to the same sequence of tokens as required. My leaning is that no action is required here. (Also, C++ programmers should stop putting so much code in inline functions in headers. That's why C++ programs are so gigantic and take ridiculous amounts of time to build. If it's big enough to merit assertions it's almost surely too big to belong in an inline.) Rich