Message ID | CADip9gZRHbOpfAdvnYpTbC0rEUAUvMBUOo6QSB7a6PVwVVoADw@mail.gmail.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 76270 invoked by alias); 16 Jan 2019 06:34:42 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 76254 invoked by uid 89); 16 Jan 2019 06:34:41 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, FROM_EXCESS_BASE64, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=Hx-languages-length:1252, H*F:D*ru X-HELO: mail-it1-f175.google.com Received: from mail-it1-f175.google.com (HELO mail-it1-f175.google.com) (209.85.166.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 16 Jan 2019 06:34:38 +0000 Received: by mail-it1-f175.google.com with SMTP id h65so1288006ith.3 for <gdb-patches@sourceware.org>; Tue, 15 Jan 2019 22:34:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=frtk-ru.20150623.gappssmtp.com; s=20150623; h=mime-version:from:date:message-id:subject:to; bh=kSL/GDZ2NLu+5bM2RODsn5e/iuoR2O8STyq7w80YzbY=; b=nG3jBx9TWBJ8UulzsLXGTscNHNRunlSxK6YQBNMjZkqCMWgNmyRR1CMFtGFGwSMLbh 6Rsuf1ao8Rso+bJZ75gmXSKiy9lLZZtReryvlpIz9SLvI4uys3leB8Z9xUhlC8ZCfmXh iU14VC0XUaphKcUX3he5kuPKtkFBfNLaVTyouOTAlotB840aoxYeNU5EKLBpX2FMf249 H/BVJ+zMGyYpQt7MZOt95bk/asFAwXsgQ7vSuM7YXTocp2PX6KGhIr9o8p3S/I3tGkHx YljGXXviJ1XJMxgdHUbtJtaFg9SR3SRw8rjey7i5JnisVMpDwcuXPFM9OYFnGXyV0TcP uCVw== MIME-Version: 1.0 From: =?UTF-8?B?0J/QsNCy0LXQuyDQmtGA0Y7QutC+0LI=?= <kryukov@frtk.ru> Date: Wed, 16 Jan 2019 09:34:25 +0300 Message-ID: <CADip9gZRHbOpfAdvnYpTbC0rEUAUvMBUOo6QSB7a6PVwVVoADw@mail.gmail.com> Subject: [PATCH] Do not expand macros to 'defined' To: gdb-patches@sourceware.org, Simon Marchi <simon.marchi@polymtl.ca> Content-Type: text/plain; charset="UTF-8" |
Commit Message
Павел Крюков
Jan. 16, 2019, 6:34 a.m. UTC
Expanding a macro which contains 'defined' PP keyword is UB. sim/common/Changelog: 2019-01-16 Pavel I. Kryukov <kryukov@frtk.ru> * sim-arange.c: eliminate DEFINE_NON_INLINE_P --- sim/common/sim-arange.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) @@ -280,9 +277,7 @@ sim_addr_range_delete (ADDR_RANGE *ar, address_word start, address_word end) build_search_tree (ar); } -#endif /* DEFINE_NON_INLINE_P */ - -#if DEFINE_INLINE_P +#else /* SIM_ARANGE_C_INCLUDED */ SIM_ARANGE_INLINE int sim_addr_range_hit_p (ADDR_RANGE *ar, address_word addr) @@ -301,4 +296,4 @@ sim_addr_range_hit_p (ADDR_RANGE *ar, address_word addr) return 0; } -#endif /* DEFINE_INLINE_P */ +#endif /* SIM_ARANGE_C_INCLUDED */
Comments
On 2019-01-16 01:34, Павел Крюков wrote: > Expanding a macro which contains 'defined' PP keyword is UB. > > sim/common/Changelog: > 2019-01-16 Pavel I. Kryukov <kryukov@frtk.ru> > > * sim-arange.c: eliminate DEFINE_NON_INLINE_P > --- > sim/common/sim-arange.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/sim/common/sim-arange.c b/sim/common/sim-arange.c > index 6373b742ce8..b3488ab564a 100644 > --- a/sim/common/sim-arange.c > +++ b/sim/common/sim-arange.c > @@ -32,10 +32,7 @@ along with this program. If not, see > <http://www.gnu.org/licenses/>. */ > #include <string.h> > #endif > > -#define DEFINE_INLINE_P (! defined (SIM_ARANGE_C_INCLUDED)) > -#define DEFINE_NON_INLINE_P defined (SIM_ARANGE_C_INCLUDED) > - > -#if DEFINE_NON_INLINE_P > +#ifdef SIM_ARANGE_C_INCLUDED > > /* Insert a range. */ > > @@ -280,9 +277,7 @@ sim_addr_range_delete (ADDR_RANGE *ar, > address_word start, address_word end) > build_search_tree (ar); > } > > -#endif /* DEFINE_NON_INLINE_P */ > - > -#if DEFINE_INLINE_P > +#else /* SIM_ARANGE_C_INCLUDED */ > > SIM_ARANGE_INLINE int > sim_addr_range_hit_p (ADDR_RANGE *ar, address_word addr) > @@ -301,4 +296,4 @@ sim_addr_range_hit_p (ADDR_RANGE *ar, address_word > addr) > return 0; > } > > -#endif /* DEFINE_INLINE_P */ > +#endif /* SIM_ARANGE_C_INCLUDED */ The patch LGTM, I agree these macros are not really needed. But I am confused, does this actually fix your c++ build? You mentioned you were using g++ (and therefore cpp), which supposedly can handle this fine: https://gcc.gnu.org/onlinedocs/cpp/Defined.html If so, what did the error look like? Simon
> which supposedly can handle this fine Right, but not with '-Wall -Wextra -Werror' flags. These pedantic options help us (a little) keeping C++ project portable between compilers, although we do not intent to have this property on GDB integration. -- Pavel > 16 янв. 2019 г., в 23:02, Simon Marchi <simon.marchi@polymtl.ca> написал(а): > >> On 2019-01-16 01:34, Павел Крюков wrote: >> Expanding a macro which contains 'defined' PP keyword is UB. >> sim/common/Changelog: >> 2019-01-16 Pavel I. Kryukov <kryukov@frtk.ru> >> * sim-arange.c: eliminate DEFINE_NON_INLINE_P >> --- >> sim/common/sim-arange.c | 11 +++-------- >> 1 file changed, 3 insertions(+), 8 deletions(-) >> diff --git a/sim/common/sim-arange.c b/sim/common/sim-arange.c >> index 6373b742ce8..b3488ab564a 100644 >> --- a/sim/common/sim-arange.c >> +++ b/sim/common/sim-arange.c >> @@ -32,10 +32,7 @@ along with this program. If not, see >> <http://www.gnu.org/licenses/>. */ >> #include <string.h> >> #endif >> -#define DEFINE_INLINE_P (! defined (SIM_ARANGE_C_INCLUDED)) >> -#define DEFINE_NON_INLINE_P defined (SIM_ARANGE_C_INCLUDED) >> - >> -#if DEFINE_NON_INLINE_P >> +#ifdef SIM_ARANGE_C_INCLUDED >> /* Insert a range. */ >> @@ -280,9 +277,7 @@ sim_addr_range_delete (ADDR_RANGE *ar, >> address_word start, address_word end) >> build_search_tree (ar); >> } >> -#endif /* DEFINE_NON_INLINE_P */ >> - >> -#if DEFINE_INLINE_P >> +#else /* SIM_ARANGE_C_INCLUDED */ >> SIM_ARANGE_INLINE int >> sim_addr_range_hit_p (ADDR_RANGE *ar, address_word addr) >> @@ -301,4 +296,4 @@ sim_addr_range_hit_p (ADDR_RANGE *ar, address_word addr) >> return 0; >> } >> -#endif /* DEFINE_INLINE_P */ >> +#endif /* SIM_ARANGE_C_INCLUDED */ > > The patch LGTM, I agree these macros are not really needed. But I am confused, does this actually fix your c++ build? You mentioned you were using g++ (and therefore cpp), which supposedly can handle this fine: > > https://gcc.gnu.org/onlinedocs/cpp/Defined.html > > If so, what did the error look like? > > Simon
On Jan 16 2019, Simon Marchi <simon.marchi@polymtl.ca> wrote: > The patch LGTM, I agree these macros are not really needed. But I am > confused, does this actually fix your c++ build? You mentioned you were > using g++ (and therefore cpp), which supposedly can handle this fine: > > https://gcc.gnu.org/onlinedocs/cpp/Defined.html > > If so, what did the error look like? This has nothing to do with C vs. C++, both have the same wording: If the token defined is generated as a result of this replacement process or use of the defined unary operator does not match one of the two specified forms prior to macro replacement, the behavior is undefined. Andreas.
On 2019-01-16 15:48, Andreas Schwab wrote: > On Jan 16 2019, Simon Marchi <simon.marchi@polymtl.ca> wrote: > >> The patch LGTM, I agree these macros are not really needed. But I am >> confused, does this actually fix your c++ build? You mentioned you >> were >> using g++ (and therefore cpp), which supposedly can handle this fine: >> >> https://gcc.gnu.org/onlinedocs/cpp/Defined.html >> >> If so, what did the error look like? > > This has nothing to do with C vs. C++, both have the same wording: > > If the token defined is generated as a result of this replacement > process or use of the defined unary operator does not match one of > the two specified forms prior to macro replacement, the behavior is > undefined. > > Andreas. That's why I was confused, the purpose of Pavel's patch that was replaced by this one is C++-oriented: https://sourceware.org/ml/gdb-patches/2019-01/msg00241.html So in the end it looks like the issue is indeed not about C++, but about his usage of -Wextra (-Wexpansion-to-defined). Simon
On 2019-01-16 15:38, Pavel Kryukov wrote: >> which supposedly can handle this fine > > Right, but not with '-Wall -Wextra -Werror' flags. > These pedantic options help us (a little) keeping C++ project portable > between compilers, although we do not intent to have this property on > GDB integration. Thanks, that clears it up. I pushed the patch. Simon
diff --git a/sim/common/sim-arange.c b/sim/common/sim-arange.c index 6373b742ce8..b3488ab564a 100644 --- a/sim/common/sim-arange.c +++ b/sim/common/sim-arange.c @@ -32,10 +32,7 @@ along with this program. If not, see <http://www.gnu.org/licenses/>. */ #include <string.h> #endif -#define DEFINE_INLINE_P (! defined (SIM_ARANGE_C_INCLUDED)) -#define DEFINE_NON_INLINE_P defined (SIM_ARANGE_C_INCLUDED) - -#if DEFINE_NON_INLINE_P +#ifdef SIM_ARANGE_C_INCLUDED /* Insert a range. */