include: Fix assert break on C++
Needs ReviewPublic
Actions

Authored by aokblast on Thu, Jun 4, 5:37 PM.

Details

Reviewers

fuz
imp

Summary

In C++, we are unable to perform implicit conversion when an object
defines an explicit operator bool. As a result, passing VA_ARG as the
parameter to bool(bool) causes the program to fail to compile.

Second, Clang provides the -verify parameter to handle expected-error
comments. Having __sanitize_assert causes the compile error to be
evaluated twice, which in turn causes the program to fail to compile.

Finally, we have no choice but to disable the sanitization test in
assert for C++.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 73699
Build 70582: arc lint + arc unit

Event Timeline

aokblast created this revision.Thu, Jun 4, 5:37 PM

Herald added a subscriber: imp. · View Herald TranscriptThu, Jun 4, 5:37 PM

aokblast requested review of this revision.Thu, Jun 4, 5:37 PM

Harbormaster completed remote builds in B73675: Diff 179224.Thu, Jun 4, 5:37 PM

aokblast added a reviewer: fuz.Thu, Jun 4, 5:38 PM

eduardo added a subscriber: eduardo.Thu, Jun 4, 5:46 PM

imp accepted this revision.Thu, Jun 4, 7:37 PM

This revision is now accepted and ready to land.Thu, Jun 4, 7:37 PM

This is incorrect. C23 requires that assert() only takes a single parameter. Your variant removes the enforcement of this invariant. The submitter says he has a fix that I hope preserves the argument count check, let's see if he comes up with something soon.

Move C case back

This revision now requires review to proceed.Fri, Jun 5, 8:20 AM

Harbormaster completed remote builds in B73699: Diff 179265.Fri, Jun 5, 8:20 AM

In D57449#1316356, @fuz wrote:

This is incorrect. C23 requires that assert() only takes a single parameter. Your variant removes the enforcement of this invariant. The submitter says he has a fix that I hope preserves the argument count check, let's see if he comes up with something soon.

Sorry that I falsely remove the check in C. It should be back now. I am good if the original submitter has a better approach. But in clang -verify case, any check would cause double evaluation and make it break.

The ((void)0) approach works but it drops the arity check and risks silent bugs. I did this for modes prior to C++20 mainly because I wasn’t sure if there were a better way. But for C++20 and later I still believe we can craft a solid solution. I’m not available today and will likely continue tomorrow, so I’d appreciate it if you could allow me a day or two. I do my best to deliver ASAP. In the meantime, please accept my apologies for any inconvenience.

In D57449#1316367, @kfv_kfv.io wrote:

The ((void)0) approach works but it drops the arity check and risks silent bugs. I did this for modes prior to C++20 mainly because I wasn’t sure if there were a better way. But for C++20 and later I still believe we can craft a solid solution. I’m not available today and will likely continue tomorrow, so I’d appreciate it if you could allow me a day or two. I do my best to deliver ASAP. In the meantime, please accept my apologies for any inconvenience.

Hi. Thanks for your reply. Please also make sure that libcxx/test/extensions/gnu/hash/specializations.verify.cpp in llvm lit test sucessful to work while you are solving this problem.

Sure thing.

Just opened a pull request. I'd appreciate it if you could take a look. I've verified everything I'm currently aware of passes at the moment (including the previously missed contextual bool conversion.) The LLVM libc++ tests mentioned earlier by @aokblast have also passed successfully.

emaste added a subscriber: emaste.Tue, Jun 9, 2:18 PM

Revision Contents
Changeset List

Path

Size

include/

assert.h

32 lines

Diff 179265

View Options

include: Fix assert break on C++Needs ReviewPublicActions