8007770: fix name clash of assert macro in debug.hpp with libc's assert macro [Was: Re: Bug 100297 - [patch] fix name clash of assert macro in debug.hpp with libc's assert macro]

[David Holmes]

On 20/02/2013 7:56 AM, Christian Thalinger wrote:
> [David already filed an RFE for this:
>
> 8007770: fix name clash of assert macro in debug.hpp with libc's assert macro]

Yes. For the record it is only undefined behaviour if you suppress the 
assert macro and re-define it as a function - which we do not do.

John Rose also added the following to the bug report:

"I count about 15,000 occurrences of assert in our sources.
The proposed rename would therefore be a disruptive change.
Patches and change sets would no longer apply across that change, 
leading to maintenance costs, especially back-porting bug fixes.
Thus, a mere claim of "would be prudent to conform exactly to the 
standard" is not enough grounds to accept this change.

Since 'assert' macros are common programming practice, we can expect (or 
request, if necessary) that the C++ standard libraries provide an op-out 
mechanism, such as '#undef assert' or '-DNDEBUG'.

For a reference on NDEBUG, see: 
http://www.cplusplus.com/reference/cassert/assert/

(To be clear, NDEBUG does not remove the macro. Another option would be 
required, if #undef is not allowed.) "

---

If this is potentially a problem then I think we need to avoid it by 
other means - most likely by bracketing standard includes with "#undef 
assert" so that they are not affected by hotspot's definition, and our 
source is not affected by the definition from assert.h

David
-----

> On Feb 19, 2013, at 7:55 AM, Péter Radics <mitchnull at gmail.com> wrote:
>
>> Hello,
>>
>> hotspot/src/share/vm/utilities/debug.hpp defines an assert macro with two
>> parameters. This macro definition clashes with libc's assert macro from
>> <assert.h> (and to the best of my knowledge it's undefined behavior to
>> redefine
>> assert like this).
>>
>> The name clash causes build failures if any of the files using debug.hpp
>> directly or indirectly include <assert.h> (or <cassert>), too.
>>
>> This doesn't seem to happen with libstdc++ (yet?), but this error is
>> triggered if
>> trying to build with libc++ (http://libcxx.llvm.org).
>>
>> I propose changing the name of the assert macro defined in debug.hpp to
>> assert_vm (based on the fact that this macro is used solely in the vm
>> subsystem).
>>
>> I've created a set of patches and attached it to issue 100297:
>> https://bugs.openjdk.java.net/show_bug.cgi?id=100297
>>
>>
>> I believe libc++ will be used by more and more systems in the near future
>> (as far as I
>> know FreeBSD is planning to include it as the default c++ lib in a coming
>> release), so
>> it would be worthwhile fixing this issue.
>
> That's an interesting point and it's something that will bite us eventually.  I would suggest to rename it to ASSERT since it's actually a macro.
>
> -- Chris
>
>>
>> cheers,
>> mitch
>