Opened 10 months ago

Closed 10 months ago

Last modified 10 months ago

#28193 closed enhancement (implemented)

Compile-time assertion

Reported by: riastradh Owned by:
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:

Description

The enclosed patch implements a CTASSERT(condition) macro that, at the top level of a file, causes a compiler error if the constant expression condition evaluates to false. This is conciser than

#if !condition
#error Condition was false.
#endif

and applicable in situations that #if cannot handle, because CTASSERT allows any constant expressions, including, e.g., sizeof, while #if is limited to C preprocessor conditional expansion. (Conversely, CTASSERT can't be used with defined(...), so it does not subsume #if.)

nickm suggested that it should be in src/lib/cc, so I put it there. If you think it should be in a different file, go for it.

The patch uses a couple of different mechanisms to implement it, depending on what the compiler supports:

  • If C11 is available, it expands to _Static_assert(condition, #condition). Obviously if you have a C11 compiler this is the best way to do it, because it is most likely to give the best error message.
  • If any of __COUNTER__, or __INCLUDE_LEVEL__ and ___LINE__, or just __LINE__, is available, their macro values are expanded and appended to a name tor_ctassert_ which is typedef'd to an array type with negative length if the condition is false, and positive length if the condition is true. This has zero run-time overhead; the use of __COUNTER__, &c., is to attain a unique name, which is guaranteed with __COUNTER__, and highly likely with __INCLUDE_LEVEL__ and __LINE__.

Child Tickets

Attachments (1)

ctassert.patch (3.6 KB) - added by riastradh 10 months ago.
patch to implement CTASSERT macro

Download all attachments as: .zip

Change History (11)

Changed 10 months ago by riastradh

Attachment: ctassert.patch added

patch to implement CTASSERT macro

comment:1 Changed 10 months ago by nickm

Milestone: Tor: 0.3.6.x-final
Status: newneeds_review

comment:2 Changed 10 months ago by dgoulet

Milestone: Tor: 0.3.6.x-finalTor: unspecified
Reviewer: nickm

comment:3 Changed 10 months ago by nickm

  1. Do we need to wrap "x" in parentheses when we pass it it to the other macros? For example, instead of
#define CTASSERT(x) CTASSERT_EXPN(x, c, __COUNTER__)

shouldn't we say

#define CTASSERT(x) CTASSERT_EXPN((x), c, __COUNTER__)

?

  1. Are you sure ATTR_UNUSED is needed? C doesn't usually complain about unused typedefs, I think.
  1. Also, how strongly do you feel about the copyright/license block here? If you use the standard Tor one, we don't have to add a new section to LICENSE, but if you use this one, we do have to add a new section (since the license says we need to distribute it along with our binaries). I'm fine with doing that if you want us to, but it's slightly more convenient if we don't.

comment:4 Changed 10 months ago by nickm

Status: needs_reviewneeds_revision

comment:5 Changed 10 months ago by riastradh

  1. I don't think there's any situation in which you need to pass arguments through to another macro with extra parentheses around them unless the downstream macro fails to parenthesize some expression itself. x can't have commas except within balanced parentheses in CTASSERT, so it can't be confused for two macro arguments when expanding into CTASSERT_EXPN. CTASSERT_EXPN, in turn, via CTASSERT_DECL, will parenthesize x in the end.

It won't hurt, so if you want to be paranoid (which is understandable among the sharp edges of the C preprocessor) you can add the parentheses, but there's no need.

  1. GCC 4.8 enabled -Wunused-local-typedefs in -Wall by default. If you don't subscribe to this warning option, then ATTR_UNUSED is not necessary. Even if it turned out to be necessary in some scenario, the only adverse effects of failing to have it will be that the compiler yells at you, so if you want to remove it, be my guest.
  1. I put the copying notice on so there would be no questions about whether it is available under a licence acceptable for Tor without having to go through the rigmarole of signing a CLA to hand it over to the Tor Project, Inc., in case you happen to have such a process.

You are welcome to use this code under the standard Tor 3-clause BSD licence. Can also just substitute 'The NetBSD Foundation, Inc.' for my name if that would be less complicated, since you already have the same licence text and same copyright holder in the top-level LICENSE file. I don't have a preference -- I just want to minimize hassle for everyone around.

comment:6 Changed 10 months ago by riastradh

Elaborating on the unused attribute: the warning -Wunused-local-typedefs fires specifically for typedefs within a function body. If you're not comfortable relying on typedefs within a function body to work (which may be an extension to C99, don't remember offhand), then you can proscribe the use of CTASSERT inside function bodies and omit ATTR_UNUSED. If you are comfortable relying on typedefs within a function body to work, then you may want to keep the ATTR_UNUSED.

comment:7 Changed 10 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.6.x-final
Status: needs_revisionmerge_ready

okay, sounds good to me! I'll merge it later this week -- probably today.

comment:8 Changed 10 months ago by nickm

Made a github PR as https://github.com/torproject/tor/pull/486 to make sure that CI likes this.

comment:9 Changed 10 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Fixed the license again (with permission) and merged to master!

comment:10 Changed 10 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

Note: See TracTickets for help on using tickets.