function argument safety check silently optimized out in release build by clang

[risa2000]

Description

With clang 21.1.8 on macOS, or clang-cl.exe 21.1.8 on Windows the release build of unit test test-class_parser_cpp11 (tests/src/unit-class_parser.cpp) crashes with an unhandled exception (SIGSEGV on macOS, Access violation on Windows).

The reason why it crashes is that in release build the compiler optimizes out the safety check here:

json/include/nlohmann/json.hpp

Line 4132 in 515d994

if (sax == nullptr)

Normally, there would be a warning emitted by the compiler:

/Users/risa/Dev/LLVM/bin/clang++ -DDOCTEST_CONFIG_SUPER_FAST_ASSERTS -DJSON_TEST_KEEP_MACROS -DJSON_TEST_USING_MULTIPLE_HEADERS=1 -I/Users/risa/Work_OSS/_risa2000/nlohmann_json/tests/thirdparty/doctest -I/Users/risa/Work_OSS/_risa2000/nlohmann_json/tests/thirdparty/fifo_map -I/Users/risa/Work_OSS/_risa2000/nlohmann_json/out/build/arm64_RelWithDebInfo/include -I/Users/risa/Work_OSS/_risa2000/nlohmann_json/include -O2 -g -DNDEBUG -std=gnu++11 -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk -Wno-deprecated -Wno-float-equal -o tests/CMakeFiles/test-class_parser_cpp11.dir/src/unit-class_parser.cpp.o -c /Users/risa/Work_OSS/_risa2000/nlohmann_json/tests/src/unit-class_parser.cpp
In file included from /Users/risa/Work_OSS/_risa2000/nlohmann_json/tests/src/unit-class_parser.cpp:12:
/Users/risa/Work_OSS/_risa2000/nlohmann_json/include/nlohmann/json.hpp:4132:13: warning: comparison of nonnull parameter 'sax' equal to a null pointer is 'false' on first
      encounter [-Wtautological-pointer-compare]
 4132 |         if (sax == nullptr)
      |             ^~~    ~~~~~~~
/Users/risa/Work_OSS/_risa2000/nlohmann_json/tests/src/unit-class_parser.cpp:224:11: note: in instantiation of function template specialization
      'nlohmann::basic_json<>::sax_parse<const std::string &, nlohmann::detail::json_sax_dom_parser<nlohmann::basic_json<>, nlohmann::detail::iterator_input_adapter<std::__wrap_iter<const char
      *>>>>' requested here
  224 |     json::sax_parse(s, &sdp);
      |           ^
/Users/risa/Work_OSS/_risa2000/nlohmann_json/include/nlohmann/json.hpp:4118:5: note: declared 'nonnull' here
 4118 |     JSON_HEDLEY_NON_NULL(2)
      |     ^
/Users/risa/Work_OSS/_risa2000/nlohmann_json/include/nlohmann/thirdparty/hedley/hedley.hpp:1287:54: note: expanded from macro 'JSON_HEDLEY_NON_NULL'
 1287 |     #define JSON_HEDLEY_NON_NULL(...) __attribute__((__nonnull__(__VA_ARGS__)))
      |                                                      ^
In file included from /Users/risa/Work_OSS/_risa2000/nlohmann_json/tests/src/unit-class_parser.cpp:12:
/Users/risa/Work_OSS/_risa2000/nlohmann_json/include/nlohmann/json.hpp:4132:13: warning: comparison of nonnull parameter 'sax' equal to a null pointer is 'false' on first
      encounter [-Wtautological-pointer-compare]
 4132 |         if (sax == nullptr)
      |             ^~~    ~~~~~~~
/Users/risa/Work_OSS/_risa2000/nlohmann_json/tests/src/unit-class_parser.cpp:251:25: note: in instantiation of function template specialization
      'nlohmann::basic_json<>::sax_parse<const std::string &, (anonymous namespace)::SaxEventLogger>' requested here
  251 |     CHECK_NOTHROW(json::sax_parse(s, &el, json::input_format_t::json, false));
      |                         ^
/Users/risa/Work_OSS/_risa2000/nlohmann_json/tests/thirdparty/doctest/doctest.h:2976:50: note: expanded from macro 'CHECK_NOTHROW'
 2976 | #define CHECK_NOTHROW(...) DOCTEST_CHECK_NOTHROW(__VA_ARGS__)
      |                                                  ^
/Users/risa/Work_OSS/_risa2000/nlohmann_json/tests/thirdparty/doctest/doctest.h:2544:77: note: expanded from macro 'DOCTEST_CHECK_NOTHROW'
 2544 | #define DOCTEST_CHECK_NOTHROW(...) DOCTEST_ASSERT_NOTHROW(DT_CHECK_NOTHROW, __VA_ARGS__)
      |                                                                             ^
/Users/risa/Work_OSS/_risa2000/nlohmann_json/tests/thirdparty/doctest/doctest.h:2521:34: note: expanded from macro 'DOCTEST_ASSERT_NOTHROW'
 2521 |             DOCTEST_CAST_TO_VOID(__VA_ARGS__)                                                      \
      |                                  ^
/Users/risa/Work_OSS/_risa2000/nlohmann_json/tests/thirdparty/doctest/doctest.h:2154:23: note: expanded from macro 'DOCTEST_CAST_TO_VOID'
 2154 |     static_cast<void>(__VA_ARGS__);                                                                \
      |                       ^
/Users/risa/Work_OSS/_risa2000/nlohmann_json/include/nlohmann/json.hpp:4118:5: note: declared 'nonnull' here
 4118 |     JSON_HEDLEY_NON_NULL(2)
      |     ^
/Users/risa/Work_OSS/_risa2000/nlohmann_json/include/nlohmann/thirdparty/hedley/hedley.hpp:1287:54: note: expanded from macro 'JSON_HEDLEY_NON_NULL'
 1287 |     #define JSON_HEDLEY_NON_NULL(...) __attribute__((__nonnull__(__VA_ARGS__)))
      |                                                      ^

but the warning -Wtautological-pointer-compare is silenced in the library code, and it seems this warning is actually trying to warn about this.

The debug build does not exhibit this problem as there the check is compiled fine.

I am not sure if this is a bug in compiler (though it seems the behavior is there since quite some time) which is too ambitious, but there is a compiler flag, which disables this optimization even in the release build -fno-delete-null-pointer-checks (supposedly available from clang 7), but the problem is, the library client must add this flag manually when using the library code.

There seems to be the similar warning (treatment) for gcc, but I do not have a gcc environment to test, if this warning is harmless, or produces the same problem.

I guess the best solution would be rewriting the code in a way it does not fall for this optimization, but so far I did not find how or if possible at all (I am not that much familiar with clang).

It is also strange that it does not get caught by CI/CD tests.

Reproduction steps

Run the release build of test-class_parser_cpp11 unit test, built with the recent version (21.1.8) of clang (on macOS), or clang-cl.exe (on Windows).

Expected vs. actual results

Should not crash.

Minimal code example

Error messages

[doctest] doctest version is "2.4.12"
[doctest] run with "--help" for options
===============================================================================
/Users/risa/Work_OSS/_risa2000/nlohmann_json/tests/src/unit-class_parser.cpp:343:
TEST CASE:  parser class
  improve test coverage
  SAX parser
  null sax handler

/Users/risa/Work_OSS/_risa2000/nlohmann_json/tests/src/unit-class_parser.cpp:343: FATAL ERROR: test case CRASHED: SIGSEGV - Segmentation violation signal

Compiler and operating system

clang (macOS), clang-cl.exe (Windows), version 21.1.8.

Library version

develop

Validation

• The bug also occurs if the latest version from the develop branch is used.
• I can successfully compile and run the unit tests.

[gregmarr]

I guess the best solution would be rewriting the code in a way it does not fall for this optimization, but so far I did not find how or if possible at all (I am not that much familiar with clang).

This test came from #4873 by @nikhilreddydev in an attempt to turn a crash on a programming error into an exception, but obviously it doesn't work in this case because the compiler is looking at the annotation and removing the check. No client code should do this, so there is nothing to fix on the library side. It might be that it's just not possible to reliably test this behavior.

[risa2000]

While I agree that it is probably a lost battle on the library side, I believe that the current state (at least for clang) is unfortunate.

My understanding of what clang does is it considers the safety check code to be somewhat convoluted form of

if (this == nullptr)

and overeagerly optimizes it out. Why it does that is something I have not figured out (yet). To me it looks like a compiler bug.

On the other hand, while doing this, compiler gives a warning, which might be considered harmless (by someone not familiar with clang quirks) and what is worse the warning never gets to the user as the library code silenced that explicitly.

The consequence however is that release build produces a code without the safety check while the user which might have believed the library will throw a C++ exception and wrote the client code around it (and it even did in a debug build) ends up in process crash in release build instead.

So if rewriting the code to avoid clang optimization is not an option, the next best thing seems to me removing the safety check altogether with corresponding unit tests as well, and have the library crash consistently same way in both debug and release builds. Anything else is begging for a problem down road.

Leaving the check in and "unsilencing" the warning seems to be a bad idea either, because unless the user understands its meaning, it may look harmless again because of debug/release dichotomy and may lead to wrong conclusions either.

In short I believe something should be done - what exactly is on you.

The another topic stemming from this is CI/CD tests not catching this. I do not know if having the unit tests giving "a ok" results is because of not testing on clang or not testing the sufficiently modern version, or some other mismatch, but this might probably needs some improvement too. Again, which one, is you to say.

Finally, I took a look at MSVC on Windows behavior related to this issue and found out that release build of the tests with MSVC is doctored explicitly to disable all optimizations (using /Od flag) and it is not clear what was the reason for that. Were some tests failing due to optimization as well? It definitely skews "release build" test results.
After removing this flag and building with recent MSVC from (VS2022) the tests passed even in release build fine, but I understand it is just one isolated experience. Even as such I would prefer if this /Od flag was used selectively only on compiler version (or other combinations) which really need it. Disabling it generally for any MSVC is making the release build tests almost useless.

[gregmarr]

FYI, the behavior you are reporting now (no safety check) is EXACTLY what we had before #4873 was merged just under 6 months ago. That PR was an attempt to change the then current behavior of an access violation into a thrown exception. The only difference is that we now have a test that tries to test that it actually throws the exception, and we now have a case where it doesn't. Whether or not that thrown exception is a good thing was discussed in the PR and the related issue.

The consequence however is that release build produces a code without the safety check while the user which might have believed the library will throw a C++ exception and wrote the client code around it (and it even did in a debug build) ends up in process crash in release build instead.

The function documentation says that the pointer must not be null. The function has an annotation on it that says the pointer must not be null. When you pass null, you get either an access violation or an exception, and the thing that you are trying to do doesn't happen. If you don't explicitly handle the exception somewhere, or have a catch-and-ignore block, your program crashes. How anyone would think that it would be okay to pass null here is beyond me.

Having said that, I have no attachment to having it throw the exception, and would be fine with it being removed, but another user of the library (I am just a user as well) thought it would be better to throw an exception, and that it would be a better debugging experience to have the exception than the access violation.

[risa2000]

I am not disputing (I hope it did not come over that way) the fact that calling the function with a nullptr is a serious bug and an exception is deserved.

I guess my major complain is that the current code gives an impression to be doing something, which it does not do consistently and on top of tries to mask it.

Right now the only way to handle the situation correctly is doing the safety check before calling the library function. Which is perfectly fine with me. Then the safety check is abundant, but still adds an additional code to the library (plus the corresponding unit tests, which do not work either).

Another point is that a process crash (at least on Windows) throws a different type of exception (structured exception) which is not typically caught by a C++ pattern. (I am not sure how this works on macOS).
So even if the user somehow decides to not check the pointer for a call before and rely on the library (or the OS) to emit an exception, the client code would need two different exception handlers, each for different build (debug/release).

Anyway I guess the best would be if the author of the patch explains the original intentions. If not, I would propose simply rolling the patch back.