llvm dev - Oct 2017 - Question: Should we consider valid a variable defined #ifndef NDEBUG scope and used in assert?

Hi,

While audit our code, we found out a strange pattern in one of the LLVM headers
and we were wondering if that was expected or if it should be fixed.

Namely the problem looks like this:
#ifndef NDEBUG
  // Define some variable.
#endif

assert(/*use this variable, i.e., outside of the ifndef NDEBUG scope*/);

This happens in include/llvm/IR/ValueHandle.h for the variable Poisoned line 494

This works because when we build LLVM with assert we explicitly disable NDEBUG:
if( LLVM_ENABLE_ASSERTIONS )
  […]
  # On non-Debug builds cmake automatically defines NDEBUG, so we
  # explicitly undefine it:
  if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )
    add_definitions( -UNDEBUG )

If we use this header in a different project and thus, with potentially
different rules for macro definitions, the compiler complains with
error:   use of undeclared identifier ‘Poisoned'

I think the right thing to do is to fix the code to work without the special way
of setting the macros, but I’d like people opinions first.

Thanks,
-Quentin
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171004/a7edc1e2/attachment.html>

I think that asserts are assumed to be controlled by NDEBUG. Does the 
other project enable them in some other way?

-Krzysztof

On 10/4/2017 4:41 PM, Quentin Colombet via llvm-dev
wrote:
> Hi,
> 
> While audit our code, we found out a strange pattern in one of the LLVM 
> headers and we were wondering if that was expected or if it should be
fixed.
> 
> Namely the problem looks like this:
> #ifndef NDEBUG
>    // Define some variable.
> #endif
> 
> assert(/*use this variable, i.e., outside of the ifndef NDEBUG scope*/);
> 
> This happens in include/llvm/IR/ValueHandle.h for the variable Poisoned 
> line 494
> 
> This works because when we build LLVM with assert we explicitly disable 
> NDEBUG:
> if( LLVM_ENABLE_ASSERTIONS )
>    […]
>    # On non-Debug builds cmake automatically defines NDEBUG, so we
>    # explicitly undefine it:
>    if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )
>      add_definitions( -UNDEBUG )
> 
> If we use this header in a different project and thus, with potentially 
> different rules for macro definitions, the compiler complains with
> error:   use of undeclared identifier ‘Poisoned'
> 
> I think the right thing to do is to fix the code to work without the 
> special way of setting the macros, but I’d like people opinions first.
> 
> Thanks,
> -Quentin
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> 
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation

I thought NDEBUG was what the assert macro also used to decide whether to
be an assert or not.

~Craig

On Wed, Oct 4, 2017 at 2:41 PM, Quentin Colombet via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hi,
>
> While audit our code, we found out a strange pattern in one of the LLVM
> headers and we were wondering if that was expected or if it should be
fixed.
>
> Namely the problem looks like this:
> #ifndef NDEBUG
>   // Define some variable.
> #endif
>
> assert(/*use this variable, i.e., outside of the ifndef NDEBUG scope*/);
>
> This happens in include/llvm/IR/ValueHandle.h for the variable Poisoned
line
> 494
>
> This works because when we build LLVM with assert we explicitly disable
> NDEBUG:
> if( LLVM_ENABLE_ASSERTIONS )
>   […]
>   # On non-Debug builds cmake automatically defines NDEBUG, so we
>   # explicitly undefine it:
>   if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )
>     add_definitions( -UNDEBUG )
>
> If we use this header in a different project and thus, with potentially
> different rules for macro definitions, the compiler complains with
> error:   use of undeclared identifier ‘Poisoned'
>
> I think the right thing to do is to fix the code to work without the
> special way of setting the macros, but I’d like people opinions first.
>
> Thanks,
> -Quentin
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171004/e49bd071/attachment.html>

Good point, I forgot to check the standard.
Given the clang was failing I assumed the code was wrong x).

I am guessing there is something weird with the project, because indeed,
paragraph 7.2 of the standard says:
The assert macro is redefined according to the current state of NDEBUG each time
that

<assert.h> is included. 

> On Oct 4, 2017, at 2:53 PM, Craig Topper <craig.topper at gmail.com>
wrote:
> 
> I thought NDEBUG was what the assert macro also used to decide whether to
be an assert or not.
> 
> ~Craig
> 
> On Wed, Oct 4, 2017 at 2:41 PM, Quentin Colombet via llvm-dev <llvm-dev
at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> Hi,
> 
> While audit our code, we found out a strange pattern in one of the LLVM
headers and we were wondering if that was expected or if it should be fixed.
> 
> Namely the problem looks like this:
> #ifndef NDEBUG
>   // Define some variable.
> #endif
> 
> assert(/*use this variable, i.e., outside of the ifndef NDEBUG scope*/);
> 
> This happens in include/llvm/IR/ValueHandle.h for the variable Poisoned
line 494
> 
> This works because when we build LLVM with assert we explicitly disable
NDEBUG:
> if( LLVM_ENABLE_ASSERTIONS )
>   […]
>   # On non-Debug builds cmake automatically defines NDEBUG, so we
>   # explicitly undefine it:
>   if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )
>     add_definitions( -UNDEBUG )
> 
> If we use this header in a different project and thus, with potentially
different rules for macro definitions, the compiler complains with
> error:   use of undeclared identifier ‘Poisoned'
> 
> I think the right thing to do is to fix the code to work without the
special way of setting the macros, but I’d like people opinions first.
> 
> Thanks,
> -Quentin
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> 
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171004/ae479957/attachment.html>