Add ALWAYS and NEVER macros (from SQLite) by isaacbrodsky · Pull Request #720 · uber/h3

isaacbrodsky · 2022-10-25T01:23:17Z

Adds ALWAYS, NEVER, and related macros from SQLite (as linked in the code comments) to help ensure fuzzers or unit tests that reach expected-unreachable code appropriately assert. This also excludes defensive code blocks from coverage metrics.

coveralls · 2022-10-25T01:27:02Z

Coverage decreased (-0.4%) to 98.597% when pulling 98a747a on isaacbrodsky:always-macro into da5b21f on uber:master.

nrabinowitz

This looks great, I find the macros very readable and clear. Just to confirm:

Using the NEVER or ALWAYS macros on a branch condition will properly exclude that branch from coverage?
The drop in coverage in this PR is due to the macros exposing reachable branches we had previously excluded from coverage?

nrabinowitz · 2022-10-25T18:00:14Z

 endif()

-option(ENABLE_COVERAGE "Enable compiling tests with coverage." ON)
+option(ENABLE_COVERAGE "Enable compiling tests with coverage." OFF)


For my info, why toggle this?

Oh, I see - this now changes the production build as well? So locally, we should make sure to toggle this ON when developing, right?

I believe this should be OFF while developing in order to get the benefit of the assertions.

nrabinowitz · 2022-10-25T18:01:14Z

+void *H3_MEMORY(malloc)(size_t size);
+void *H3_MEMORY(calloc)(size_t num, size_t size);
+void *H3_MEMORY(realloc)(void *ptr, size_t size);
+void H3_MEMORY(free)(void *ptr);


Why wasn't this previously formatted via clang?

It was not included in the list of source files, presumably an oversight.

nrabinowitz · 2022-10-25T18:02:30Z

+ *
+ *    May you do good and not evil.
+ *    May you find forgiveness for yourself and forgive others.
+ *    May you share freely, never taking more than you give.


This is amazing 😮

nrabinowitz · 2022-10-25T18:07:43Z

+#define NEVER(X) (0)
+#elif !defined(NDEBUG)
+#define ALWAYS(X) ((X) ? 1 : (assert(0), 0))
+#define NEVER(X) ((X) ? (assert(0), 1) : 0)


So I understand, the expression (assert(0), 1) means: first assert 0 (i.e. fail), then return 1, which is unused in practice but necessary for the compiler?

Conceptually that is correct.

nrabinowitz · 2022-10-25T18:09:42Z

                // Should not be possible because `origin` would have to be a
                // pentagon
-                return neighborResult;  // LCOV_EXCL_LINE
+                // TODO: Reachable via fuzzer


For my info: Were these TODOs uncovered by trying to use the new macros here and triggering assertion errors?

Yes, I generally tried to replace LCOV exclusions first with the macros and had to replace them with TODOs, since this PR makes it clear they are reachable.

nrabinowitz · 2022-10-25T18:11:48Z

    int oldBaseCell = H3_GET_BASE_CELL(current);
-    if (oldBaseCell < 0 ||  // LCOV_EXCL_BR_LINE
-        oldBaseCell >= NUM_BASE_CELLS) {
+    if (NEVER(oldBaseCell < 0) || oldBaseCell >= NUM_BASE_CELLS) {


For my info: This branch is now considered covered in our coverage metrics?

Yes, because for coverage it just sees 0 || oldBaseCell >= NUM_BASE_CELLS (constant folds to oldBaseCell >= NUM_BASE_CELLS, which is covered).

nrabinowitz · 2022-10-25T18:14:50Z

                break;
            }
        }
+        if (p == NUM_PENTAGONS) {


Should this be NEVER(p == NUM_PENTAGONS)? This case is supposedly unreachable, right?

I'm not sure that would satisfy the compile error about dirFaces being uninitialized. We could check.

Co-authored-by: Nick Rabinowitz <[email protected]>

isaacbrodsky · 2022-10-25T14:55:22Z

 #define ALLOC_H

-#include "h3api.h" // for TJOIN
+#include "h3api.h"  // for TJOIN


This file was incorrectly not formatted before

isaacbrodsky · 2022-10-25T23:13:46Z

 endif()

-option(ENABLE_COVERAGE "Enable compiling tests with coverage." ON)
+option(ENABLE_COVERAGE "Enable compiling tests with coverage." OFF)


I believe this should be OFF while developing in order to get the benefit of the assertions.

isaacbrodsky · 2022-10-25T23:14:09Z

+void *H3_MEMORY(malloc)(size_t size);
+void *H3_MEMORY(calloc)(size_t num, size_t size);
+void *H3_MEMORY(realloc)(void *ptr, size_t size);
+void H3_MEMORY(free)(void *ptr);


It was not included in the list of source files, presumably an oversight.

isaacbrodsky · 2022-10-25T23:17:26Z

+#define NEVER(X) (0)
+#elif !defined(NDEBUG)
+#define ALWAYS(X) ((X) ? 1 : (assert(0), 0))
+#define NEVER(X) ((X) ? (assert(0), 1) : 0)


Conceptually that is correct.

isaacbrodsky · 2022-10-25T23:18:51Z

    int oldBaseCell = H3_GET_BASE_CELL(current);
-    if (oldBaseCell < 0 ||  // LCOV_EXCL_BR_LINE
-        oldBaseCell >= NUM_BASE_CELLS) {
+    if (NEVER(oldBaseCell < 0) || oldBaseCell >= NUM_BASE_CELLS) {


Yes, because for coverage it just sees 0 || oldBaseCell >= NUM_BASE_CELLS (constant folds to oldBaseCell >= NUM_BASE_CELLS, which is covered).

isaacbrodsky · 2022-10-25T23:20:30Z

                // Should not be possible because `origin` would have to be a
                // pentagon
-                return neighborResult;  // LCOV_EXCL_LINE
+                // TODO: Reachable via fuzzer


Yes, I generally tried to replace LCOV exclusions first with the macros and had to replace them with TODOs, since this PR makes it clear they are reachable.

isaacbrodsky · 2022-10-25T23:21:57Z

                break;
            }
        }
+        if (p == NUM_PENTAGONS) {


I'm not sure that would satisfy the compile error about dirFaces being uninitialized. We could check.

isaacbrodsky · 2022-10-25T23:24:36Z

This looks great, I find the macros very readable and clear. Just to confirm:
* Using the `NEVER` or `ALWAYS` macros on a branch condition will properly exclude that branch from coverage?

Yes, you can find an example here: https://coveralls.io/builds/53603547/source?filename=src%2Fh3lib%2Flib%2FdirectedEdge.c#L282

* The drop in coverage in this PR is due to the macros exposing reachable branches we had previously excluded from coverage?

Yes, I had hoped this PR would increase coverage by being able to exclude some more branches we had not marked with exclusions, but rather it exposed they are reachable, and we should cover them via unit testing.

ajfriend · 2022-11-07T03:55:49Z

 ## ENABLE_COVERAGE

-Whether to compile `Debug` builds with coverage instrumentation (compatible with GCC, Clang, and lcov).
+Whether to compile `Debug` builds with coverage instrumentation (compatible with GCC, Clang, and lcov). This also elides defensive code in the library.


I had to look up "elides". Maybe "bypasses"? 😅

And maybe even a quick note on why:

"This also bypasses defensive code in the library, since it should typically not be reachable by coverage."

ajfriend · 2022-11-07T04:19:43Z

+## Defensive code
+
+H3 uses preprocessor macros borrowed from [SQLite's testing methodology](https://www.sqlite.org/testing.html) to include defensive code in the library. Defensive code is code that handles error conditions for which there are no known test cases to demonstrate it. The lack of known test cases means that without the macros, the defensive cases could inappropriately reduce coverage metrics, disincentivizing including them.
+


nit/suggestion: What about presenting the next three paragraphs as an unordered list, introduced by something like

The macros will behave differently, depending on the build type being "release", "debug", or "coverage":

It is probably a matter of personal choice, but I find it can make documentation a little more scannable.

One styling idea:

The macros will behave differently, depending on the library build type being "release", "debug", or "coverage":

release builds: The defensive code is included without modification. These branches are intended to be very simple (usually only returning an error code and possibly freeing some resources) and to be visually inspectable.

debug builds: The defensive code is included and assert calls are included if the defensive code is invoked. Any unit test or fuzzer which can demonstrate the defensive code is actually reached will trigger a test failure and the developers can be alerted to cover the defensive code in unit tests.

coverage builds: The defensive code is not included by replacing its condition with a constant value. The compiler removes the defensive code and it is not counted in coverage metrics.

ajfriend · 2022-11-07T04:24:26Z

These macros are awesome!

ajfriend · 2022-11-07T19:57:02Z

@isaacbrodsky, this is maybe more of a conceptual question, but what if there were a user who was risk-tolerant but very interested in performance. Would it make sense for them to want to build a release version of the library that bypasses the defensive code (like we do in the coverage build)? Is that configuration possible, or should we allow for that?

Would that provide any potential performance benefit by skipping the defensive checks?

isaacbrodsky · 2022-11-08T00:45:12Z

@isaacbrodsky, this is maybe more of a conceptual question, but what if there were a user who was risk-tolerant but very interested in performance. Would it make sense for them to want to build a release version of the library that bypasses the defensive code (like we do in the coverage build)? Is that configuration possible, or should we allow for that?

Would that provide any potential performance benefit by skipping the defensive checks?

I don't think that configuration is supported here. It would be possible to eliminate some conditional checks, but I would imagine the performance gain to be modest. We could add another CMake option that controls whether those are included or not that's separate from (probably defaulted from) the coverage option.

ajfriend · 2022-11-08T20:05:52Z

I don't think that configuration is supported here. It would be possible to eliminate some conditional checks, but I would imagine the performance gain to be modest. We could add another CMake option that controls whether those are included or not that's separate from (probably defaulted from) the coverage option.

Makes sense. And I don't think we need to make any changes here. That was just a conceptual question.

isaacbrodsky added 5 commits October 24, 2022 15:01

Add ALWAYS macro from Sqlite

6e21577

building with ALWAYS/NEVER

34c109a

Add more NEVER macros

96dae14

Docs update

3f9935e

CHANGELOG

1e5e16b

isaacbrodsky requested review from ajfriend, dfellis and nrabinowitz October 25, 2022 01:23

Add PR number

a940ebe

isaacbrodsky added 12 commits October 24, 2022 19:03

Fix formatting

c09a4b0

Fix compiler warnings, fuzzer reachable case

f7568cc

Fix fuzzer reachable case

da6bbd0

Back out ALWAYS in h3NeighborRotations

c89c033

Back out more reachable cases

6a3dcc3

Back out more reachable cases (2)

ae6a3f3

Back out more reachable cases (3)

20251d9

Back out more reachable cases (4)

6abf4c7

Back out more reachable cases (5)

69865a5

Back out more reachable cases (6)

ea17da7

Back out more reachable cases (7)

d1dcd0a

Back out more reachable cases (8)

963fbbb

nrabinowitz approved these changes Oct 25, 2022

View reviewed changes

nrabinowitz mentioned this pull request Oct 25, 2022

Feature: Add cellToChildPos and childPosToCell #719

Merged

isaacbrodsky and others added 2 commits October 25, 2022 16:16

Update src/h3lib/include/h3Assert.h

d6ff321

Co-authored-by: Nick Rabinowitz <[email protected]>

Update website/docs/core-library/testing.md

184836b

Co-authored-by: Nick Rabinowitz <[email protected]>

isaacbrodsky commented Oct 25, 2022

View reviewed changes

ajfriend reviewed Nov 7, 2022

View reviewed changes

ajfriend approved these changes Nov 7, 2022

View reviewed changes

Docs update per review

98a747a

isaacbrodsky force-pushed the always-macro branch from 77cad8f to 98a747a Compare November 8, 2022 20:35

isaacbrodsky merged commit fc5a3c8 into uber:master Nov 8, 2022

isaacbrodsky deleted the always-macro branch November 8, 2022 21:08

		## Defensive code

		H3 uses preprocessor macros borrowed from [SQLite's testing methodology](https://www.sqlite.org/testing.html) to include defensive code in the library. Defensive code is code that handles error conditions for which there are no known test cases to demonstrate it. The lack of known test cases means that without the macros, the defensive cases could inappropriately reduce coverage metrics, disincentivizing including them.

Conversation

isaacbrodsky commented Oct 25, 2022

Uh oh!

coveralls commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrabinowitz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isaacbrodsky commented Oct 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajfriend commented Nov 7, 2022

Uh oh!

ajfriend commented Nov 7, 2022

Uh oh!

isaacbrodsky commented Nov 8, 2022

Uh oh!

ajfriend commented Nov 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Oct 25, 2022 •

edited

Loading