diff mbox

Do not expand macros to 'defined'

Message ID CADip9gZRHbOpfAdvnYpTbC0rEUAUvMBUOo6QSB7a6PVwVVoADw@mail.gmail.com
State New, archived
Headers

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

Simon Marchi Jan. 16, 2019, 8:02 p.m. UTC | #1 
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
Павел Крюков Jan. 16, 2019, 8:38 p.m. UTC | #2 
> 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
Andreas Schwab Jan. 16, 2019, 8:48 p.m. UTC | #3 
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.
Simon Marchi Jan. 16, 2019, 8:58 p.m. UTC | #4 
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
Simon Marchi Jan. 16, 2019, 9:11 p.m. UTC | #5 
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 mbox

Patch

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.  */