Page MenuHomeFreeBSD

Remove potential identifier conflict in the EV_SET macro.
ClosedPublic

Authored by dab on Jun 6 2018, 7:01 PM.

Details

Summary

PR43905 pointed out a problem with the EV_SET macro if the
passed struct kevent pointer were specified with an expression with
side effects (e.g., "kevp++"). This was fixed in rS110241, but by
using a local block that defined an internal variable (named "kevp")
to get the pointer value once. This worked, but could cause issues if
an existing variable named "kevp" is in scope. To avoid that issue,
@jilles pointed out that "C99 compound literals and designated
initializers allow doing this cleanly using a macro". This change
incorporates that suggestion, essentially verbatim from @jilles
comment on PR43905.

I should note that the goal of making this change is primarily to remove the last (potential) open issue in PR43905 and close it. One could certainly make the argument that the existing solution is sufficient and the bug should just be closed as-is. I'm open to hearing such an argument.

PR: 43905

Test Plan

Run the kqueue system tests.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 17181
Build 17032: arc lint + arc unit

Event Timeline

dab created this revision.Jun 6 2018, 7:01 PM
dab added inline comments.Jun 6 2018, 7:05 PM
sys/sys/event.h
52

I could have done this as just a straight replacement for the old definition as from what I understand all platforms compile with at least C99 support. However, I decided to take this approach in case someone is still building with a pre-C99 compiler. I saw several other places in the code base that check for C99/pre-C99.

dab edited the test plan for this revision. (Show Details)Jun 6 2018, 7:10 PM
dab edited the summary of this revision. (Show Details)Jun 6 2018, 7:16 PM
jilles requested changes to this revision.Jun 6 2018, 9:41 PM

Thanks for working on this, but I think pedantic compilation modes should be kept working.

sys/sys/event.h
52

Supporting pre-C99 here is not immediately unreasonable, since this file is included in user code.

Apart from that, standard C++ does not support compound literals, so using them may cause a warning. Both GCC and Clang support compound literals in C++ as an extension, but with different semantics if a pointer is taken to one (which does not happen here).

There should be a check defined(__STDC_VERSION__) so that compiling C++ code with -Wundef does not generate a warning.

This revision now requires changes to proceed.Jun 6 2018, 9:41 PM
dab updated this revision to Diff 43397.Jun 7 2018, 1:11 AM

Add check for STDC_VERSION being defined before attempting to check its value.

dab marked 2 inline comments as done.Jun 7 2018, 1:13 AM
dab updated this revision to Diff 43621.Jun 11 2018, 9:18 PM

Revert an unintended change to the original EV_SET macro that crept into my last revision.

dab added a subscriber: vangyzen.Jun 20 2018, 9:29 PM

@jilles, @jmg : Any further input on this change?

jmg added a comment.Jun 25 2018, 11:22 PM

no comments...

jilles accepted this revision.Jun 26 2018, 8:32 PM

Looks good to me apart from the comment. Sorry for the delay.

sys/sys/event.h
67

Pre-C99 or C++

This revision is now accepted and ready to land.Jun 26 2018, 8:32 PM