[LLVMify F18] Use llvm_unreachable for unreachable code cases

[DavidTruby]

llvm_unreachable expands to an assert in debug mode and a compiler optimization hint abstracted across the supported compilers in release mode. This allows additional compiler optimizations based on this assumption. We should replace our uses of assert(false), DIE("unreachable") and similar with llvm_unreachable to take advantage of this.

[klausler]

There are no instances of assert(false) or CHECK(false) in f18. There's 9 instances of die("unreachable") or DIE("unreachable"), all in the same file. I recommend replacement with a macro, which can be defined alongside the macro CRASH_NO_CASE.

[DavidTruby]

Why define our own macro when something suitable already exists elsewhere in the project?

[klausler]

Because, as mentioned above, you'll probably also want to modify CRASH_NO_CASE.

[kiranktp]

llvm_unreachable will always print the filename and the line number of the problematic location.

llvm_unreachable("..") is a drop in replacement for DIE(".."). so different cases like CRASH_NO_CASE, SWITCH_COVERS_ALL_CASES and CHECK can leverage llvm_unreachable(..) similar to DIE("..).

But Fortran::common::die accepts varargs. This honors the format specifiers. In all the case this function will not print the filename and the line number. Ex: common::die("missing case to fold intrinsic function %s", name.c_str()) The error we get is: fatal internal error: missing case to fold intrinsic function xyz

Should we replace this function also to use llvm_unreachable or can we have f18 specific implementation for this scenario?

Either ways, for the above case we need to have f18 specific implementation to honor format specifiers, as llvm_unreachable does not honor format specifier. Please advice. Correct me if my understanding is incorrect.

[DavidTruby]

Hi Kiran, thanks for looking at this. At a glance I think we can replace our die function with something like the following (I haven't tried to compile this code so it may be nonsense):

template <typename Args...> 
[[noreturn]] void die(const char* c, Args &... args) {
   std::string buf;
   llvm::raw_string_ostream ss {buf};
   buf << llvm::format(c, args...);
   llvm_unreachable(buf.str());
}

and then get rid of our DIE macro. Do you think something like this could work?

As regards the other point, I can't see why the file and line information wouldn't always be useful, so I don't think we need the distinction between the two.

[kiranktp]

Thanks @DavidTruby. Your code was almost perfect. :-) I have replaced DIE with llvm_unreachable and raised a pull request.

[sscalpone]

We must not conflate assertion checks with unreachable code in llvm because llvm makes a distinction between assert and llvm_unreachable.

See https://llvm.org/docs/CodingStandards.html#id42

Use llvm_unreachable to mark a specific point in code that should never be reached.

LLVM disables the assert and llvm_unreachable checks in RELEASE builds. As far as I know, re-enabling assert in a RELEASE build requires a bit of ingenuity.

I think it is better not to use assert directly in the source, but instead to have a function call or different macro as an intermediary to assist debugging of RELEASE builds.

[DavidTruby]

I believe CMake's RelWithDebugInfo build type leaves assets on? That should help with debugging optimised builds if so.

The advantage of using llvm_unreachable and assert is they both use compiler specific macros/intrinsics to signal Undefined Behaviour. This allows extra compiler optimisations in some cases (e.g. not bothering to generate code for an unreachable case, as an obvious example)

[DavidTruby]

What I said seems to be untrue, RelWithDebInfo still has -DNDEBUG. However, LLVM does have an LLVM_ENABLE_ASSERTIONS variable in cmake to force both assert and llvm_unreachable on in any build configuration, and that can be used in conjunciton with Release builds.

[sscalpone]

The idea is to enable flang assertion checking without enabling everything that controlled by NDEBUG.

[sscalpone]

Also, unless potential undefined behavior is intended, we ought to leave assertions and checks for unreachable code enabled all the time. We might consider adopting something like Rust's debug_assert.

[DavidTruby]

Doesn't debug_assert in Rust do the same thing as assert here? As in, it gets disabled in optimised/release builds?

It's not so much that potential UB is intended as it is that it allows extra interesting optimisations for the compiler to take advantage of. I don't see a reason we wouldn't want these optimisations to occur in Release/optimised builds?

[sscalpone]

Yes, Rust's debug_assert does what std::assert does and Rust's assert is checked for both debug and release builds.

Assuming that we want the assertion checks, what the issue with optz?

[tskeith]

CHECK and DIE are intended to be enabled in release builds. std::assert and llvm_unreachable are available for when you want them disabled in release builds.

To change existing use of the former to the latter we need to make the case that the performance and size benefits are greater than the debugging benefits.

[DavidTruby]

Do we need users to be able to use these for debugging? Because, as developers we could just build with -DLLVM_ENABLE_ASSERTIONS and test with that in release mode (which is what we already do in our CI downstream for what it's worth)

[tskeith]

Do we need users to be able to use these for debugging?

We want them enabled so that when users hit them they get a clear indication that it is an internal error and information that is helpful to us about where it happened.

[DavidTruby]

Ah, I see what you mean now. I agree that there's a difference between assert and unreachable code, and also that there's a difference between something we want to use assert for and an ICE that a user might hit. However, I think that a lot of the cases in the PR that @kiranktp had up before fall into the former category.

We're currently lumping both "function preconditions that we expect tests to catch", "actually unreachable code" and "Possible ICEs" into the same category with the CHECK/DIE macros. I think rather we should consider using assert for the first category, llvm_unreachable for the second and some more robust mechanism for reporting ICEs for the third.

Perhaps we should look at how clang reports ICEs? It provides nice stack traces and more diagnostic info for those cases.

[joker-eph]

CHECK and DIE are intended to be enabled in release builds

LLVM has report_fatal_error for this, does it match your "DIE" use case?

We want them enabled so that when users hit them they get a clear indication that it is an internal error and information that is helpful to us about where it happened.

Anyone can ship an LLVM toolchain with LLVM_ENABLE_ASSERTIONS=ON ;)

[dwblaikie]

So, LLVM's philosophy here is: 1) if this is an invariant that represents a programmer error (something that should be fixed by changing some source code - either inside or (if using LLVM as a library) outside LLVM) if it fails - use assert and llvm_unreachable (llvm_unreachable is described in the LLVM developer guide as "a better assert(false)") 2) if this is a runtime error meant to be handled by code/users (or users that are code - if they're using parts of LLVM as a library) it should use the usual error handling such as llvm::Error 3) report_fatal_error is sort of a dodgy stop-gap. It's for errors where someone finds it too onerous to do (2) but they think it might actually happen - but it hard stops the program. This isn't appropriate error handling for a library which LLVM and its subprojects strive to be

This is something I would have some pretty strong feelings about with regard to flang merging into the LLVM project: Please assert/unreachable your preconditions. If users experience crashes, having report_fatal_error won't make things any better for them - compiler engineers will take their reproduction and use a +Asserts build (as mentioned - it doesn't require any particular ingenuity to enable them - LLVM_ENABLE_ASSERTIONS=ON - yes, anyone developing code inside LLVM or using LLVM as a library should be using a build with assertions enabled) to get more info as the first step in debugging.

[DavidTruby]

@dwblaikie thanks for your advice here! I am willing to commit myself to take a much deeper look at error handling as a first priority post-merge; but I think it'll be much easier to do then than before merging. Once we're on the same infrastructure it will be easier to attract reviewers from the whole LLVM community, and getting changes committed in line with what that community wants should be a lot easier as a result. If I commit myself to that, will this be a blocker for you with regards to the merge?

[dwblaikie]

The state of the code is less important to me than the agreement of the f18/flang community - it's something I think as a community it should be clear agreement going into this.

[sscalpone]

There was an objection to replacing unreachable code with assert instead of llvm_unreachable. I believe we're on the same page about llvm_unreachable for things like unhandled cases.

There's still discussion about how to represent runtime checks that we want to always be enabled, with or without NDEBUG. NDEBUG is a big hammer that I would prefer be independent of low-cost runtime checks, like null pointer checks.

I'm fine with using NDEBUG and std::assert for checks that are expensive or otherwise undesirable when the compiler is running.

[dwblaikie]

There was an objection to replacing unreachable code with assert instead of llvm_unreachable. I believe we're on the same page about llvm_unreachable for things like unhandled cases.

There's still discussion about how to represent runtime checks that we want to always be enabled, with or without NDEBUG. NDEBUG is a big hammer that I would prefer be independent of low-cost runtime checks, like null pointer checks.

Yeah, that would be a divergence from the LLVM project that I wouldn't be comfortable with. LLVM asserts liberally for even low-cost checks like null pointers & I believe that's the right call. (LLVM does have a layer beyond asserts - "expensive checks" for the really expensive things, but even enabling the non-expensive checks is more expensive than you'd want in release builds)

[dwblaikie]

This is the LLVM style guide's wording on assertions: https://llvm.org/docs/CodingStandards.html#assert-liberally - the examples show fairly cheap tests.

[DavidTruby]

@sscalpone at least according the to the C++ standard, NDEBUG does nothing other than enable the assert macro. What other behaviour are you expecting from it that makes it a big hammer?

[tskeith]

There's still discussion about how to represent runtime checks that we want to always be enabled, with or without NDEBUG. NDEBUG is a big hammer that I would prefer be independent of low-cost runtime checks, like null pointer checks.

Yeah, that would be a divergence from the LLVM project that I wouldn't be comfortable with.

I think I'm missing something here. You're not comfortable with flang containing runtime checks in release builds? Why?

[dwblaikie]

There's still discussion about how to represent runtime checks that we want to always be enabled, with or without NDEBUG. NDEBUG is a big hammer that I would prefer be independent of low-cost runtime checks, like null pointer checks.

Yeah, that would be a divergence from the LLVM project that I wouldn't be comfortable with.

I think I'm missing something here. You're not comfortable with flang containing runtime checks in release builds? Why?

Runtime checks for invariants, yes - because it's not consistent with the LLVM style guide/way of writing code. (& because I agree with it - and I think it's important to not treat invariant violations the same as user errors - in part because testing is very different, you don't write test cases to demonstrate invariant violations, but you should have test cases for every user error, for instance)

[sscalpone]

What other behaviour are you expecting from [NDEBUG] that makes it a big hammer?

A lot of code uses NDEBUG to disable runtime checks. There are over 1300 instances in llvm-project. Not all of these are low-cost.

The Rust language deals with the issue head on: asserts are checked in debug and release builds, and cannot be disabled; debug_assert is not enabled in release builds by default. I'm not suggesting that llvm adopt these semantics for assert, but instead suggesting that flang define its own always-on assertions.

[tskeith]

I think I'm missing something here. You're not comfortable with flang containing runtime checks in release builds? Why?

Runtime checks for invariants, yes - because it's not consistent with the LLVM style guide/way of writing code.

So the requirement is for undefined behavior rather than abort with a message when an invariant is not satisfied? That makes no sense.

[dwblaikie]

Makes a fair bit of sense to the rest of the LLVM project, and I'd not be comfortable with flang being different in this regard.

The requirement is to be able to opt-in to invariant checking, not having it always-on.

[sscalpone]

The requirement is to be able to opt-in to invariant checking, not having it always-on.

@dwblaikie How would you feel if that opt-in was not directly controlled by NDEBUG?

[DavidTruby]

What other behaviour are you expecting from [NDEBUG] that makes it a big hammer?

A lot of code uses NDEBUG to disable runtime checks. There are over 1300 instances in llvm-project. Not all of these are low-cost.

I would argue this is a bug in LLVM that should be fixed if such a thing is a problem.

The Rust language deals with the issue head on: asserts are checked in debug and release builds, and cannot be disabled; debug_assert is not enabled in release builds by default. I'm not suggesting that llvm adopt these semantics for assert, but instead suggesting that flang define its own always-on assertions.

I'm of the opinion that flang should not define anything of its own where an equivalent way of doing things exists in LLVM or in LLVM's coding guidelines; we are (aspiring to be?) a part of the LLVM project and as such should do things the way the LLVM project expects them to be done.

If the NDEBUG issue is a problem, perhaps we should discuss that with the whole project post-merge? It seems to me that if an opt-in invariant mechanism separate to NDEBUG would be useful to Flang, it would surely be useful to other components of LLVM?

[joker-eph]

I think I'm missing something here. You're not comfortable with flang containing runtime checks in release builds? Why?

I am comfortable with your distribution of flang containing runtime checks: you just have to build it with -DLLVM_ENABLE_ASSERTIONS=ON

So the requirement is for undefined behavior rather than abort with a message when an invariant is not satisfied? That makes no sense.

This is how LLVM works: you will be dependent on a lot of LLVM infrastructure (and MLIR) as well that have this behavior, you better review how you view the system as a whole. I am not what makes the very frontend aspect of it different from LLVM/MLIR from this point of view.

Also, your sentence is a clear indication that you should always build with UBSAN: you want a runtime check instead of any UB right?

@dwblaikie How would you feel if that opt-in was not directly controlled by NDEBUG?

What are you trying to achieve here? NDEBUG is exactly used for this purpose in LLVM. If you want to rename NDEBUG for some reason, you should bring it up to LLVM as whole and rename every single assert in LLVM.

[joker-eph]

A lot of code uses NDEBUG to disable runtime checks. There are over 1300 instances in llvm-project. Not all of these are low-cost.

If the only concerns is that LLVM would get too slow and you want to assert only in flang, that's possible: a cmake option -DFLANG_ONLY_ASSERTIONS=ON can turn off -DNDEBUG only when building the flang codebase and not when building LLVM/MLIR.

[sscalpone]

I'm not against NDEBUG. I don't want to forestall the use of NDEBUG in flang. I don't want to ship production builds without NDEBUG.

I'd just like to allow for a low-cost runtime assertion checks in flang that are independent of NDEBUG.

I would love UBSAN if it was free :)

[DavidTruby]

I'm not sure I understand what such assertion checks would be meant to solve;

Are they meant to be recoverable user errors? Then we should be using llvm::Error and other error handling mechanisms to resolve the errors properly.

Are they meant to be function invariants? Then we should use assert, and ask users to give us reproducers/give users assertion enabled builds at that point so we can see the error.

I'm not sure I see what fits in the middle ground?

[DavidTruby]

I've had a look at how clang handles this; what clang does is uses asserts etc as usual, but the driver has code such that when one of the compilation stages it reruns it with some extra logic to find out how the crash occurred. This seems like the solution we would want to finally adopt to me, however I think we're a way off having a driver sophisticated enough to do this.

I would like to propose that in the meantime we move to using standard asserts where appropriate in non-controversial places (e.g. where @sscalpone would be happy using something like Rust's debug_assert as mentioned above); llvm_unreachable for truly unreachable cases, and llvm's report_fatal_error or similar for the cases where we actually do need line numbers reported. I can implement this post-merge as a first priority.

However, we would use this solely as a temporary solution until we can move to llvm::Error where we are reporting real user errors, and integrate something like Clang's ICE reporting code once we have a sophisticated enough driver.

Would this be acceptable to everyone as an agreed way forward so the merge can go ahead?

[tskeith]

I'm not sure I understand what such assertion checks would be meant to solve;

Are they meant to be recoverable user errors? Then we should be using llvm::Error and other error handling mechanisms to resolve the errors properly.

Are they meant to be function invariants? Then we should use assert, and ask users to give us reproducers/give users assertion enabled builds at that point so we can see the error.

I'm not sure I see what fits in the middle ground?

They are meant to be invariants. And because their cost is negligible we should leave them enabled so that users don't have to jump through extra hoops to see that they have been violated.

[DavidTruby]

They are meant to be invariants. And because their cost is negligible we should leave them enabled so that users don't have to jump through extra hoops to see that they have been violated.

The ideal solution that I'm proposing we eventually move to (something like Clang's ICE reporting code) the user doesn't have to jump through any extra hoops, it's transparent to the person running the compiler.

[tskeith]

The ideal solution that I'm proposing we eventually move to (something like Clang's ICE reporting code) the user doesn't have to jump through any extra hoops, it's transparent to the person running the compiler.

I am completely in favor of moving to something better. I am only opposed to moving to something worse in the meantime.

[DavidTruby]

I am completely in favor of moving to something better. I am only opposed to moving to something worse in the meantime.

I'm not proposing moving to something worse; llvm::report_fatal_error would be neither better nor worse than what we have now, but it would be a standard mechanism from the LLVM project. However, I'm not opposed to leaving the cases we really want invariants enabled all the time as-is now until we can get the Driver up to scratch to do the reporting properly in the way clang does.

[DavidTruby]

On our bi-weekly call earlier, @sscalpone agreed with my above comment as a way forward. @dwblaikie and @joker-eph are you ok with us going ahead with the merge tomorrow on these terms?

[sscalpone]

On our bi-weekly call earlier, @sscalpone agreed with my above comment as a way forward.

I agreed to non-controversial asserts and llvm_notreached, yes.

I don't see a problem using report_fatal_error() until the code adopts llvm::Error, if that's the recommended path.

[dwblaikie]

On our bi-weekly call earlier, @sscalpone agreed with my above comment as a way forward.

I agreed to non-controversial asserts and llvm_notreached, yes.

I don't see a problem using report_fatal_error() until the code adopts llvm::Error, if that's the recommended path.

llvm::Error isn't the recommended path for testing/reporting invariant violations (assert/unreachable is), to be clear - assert/unreachable is the tool for that. Any codepath with llvm::Error shuold be reachable/testable/tested via execution of the production binaries, or supported API surface area.

I am completely in favor of moving to something better. I am only opposed to moving to something worse in the meantime.

I'm not proposing moving to something worse; llvm::report_fatal_error would be neither better nor worse than what we have now, but it would be a standard mechanism from the LLVM project. However, I'm not opposed to leaving the cases we really want invariants enabled all the time as-is now until we can get the Driver up to scratch to do the reporting properly in the way clang does.

I think this might be do-able in advance cheaper than migrating things to llvm::report_fatal_error - well, not quite Clang's crash reporting, but LLVM's general crash reporting which provides a backtrace, line numbers (in debug builds), link to where to report bugs, program and its arguments, etc:

InitLLVM X(argc, argv);

should be what's needed. That takes a crash (in this case an assertion failure in a +Asserts build, but other crashes like segfault, etc, will behave similarly) from this:

llvm-dwarfdump-tot: /usr/local/google/home/blaikie/dev/llvm/src/llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:575: int main(int, char **): Assertion `false' failed. Aborted

to this:

llvm-dwarfdump-tot: /usr/local/google/home/blaikie/dev/llvm/src/llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:576: int main(int, char **): Assertion `false' failed. PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace. Stack dump:

0. Program arguments: llvm-dwarfdump-tot

   0 0x0000000000b69317 llvm::sys::PrintStackTrace(llvm::raw_ostream&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/Unix/Signals.inc:564:11

   1 0x0000000000b694b9 PrintStackTraceSignalHandler(void*) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/Unix/Signals.inc:625:1

   2 0x0000000000b67ccb llvm::sys::RunSignalHandlers() /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/Signals.cpp:67:5

   3 0x0000000000b69c35 SignalHandler(int) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/Unix/Signals.inc:406:1

   4 0x00007fb1e1482520 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x13520)

   5 0x00007fb1e0f55f61 raise (/lib/x86_64-linux-gnu/libc.so.6+0x39f61)

   6 0x00007fb1e0f41535 abort (/lib/x86_64-linux-gnu/libc.so.6+0x25535)

   7 0x00007fb1e0f4140f (/lib/x86_64-linux-gnu/libc.so.6+0x2540f)

   8 0x00007fb1e0f4eb92 (/lib/x86_64-linux-gnu/libc.so.6+0x32b92)

   9 0x00000000007a8c51 (/usr/local/google/home/blaikie/dev/llvm/build/default/bin/llvm-dwarfdump+0x7a8c51)

   10 0x00007fb1e0f42bbb __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26bbb)

   11 0x000000000079802a _start (/usr/local/google/home/blaikie/dev/llvm/build/default/bin/llvm-dwarfdump+0x79802a)

   Aborted

Clang-style crash reporting that creates full reproducers, etc, is certainly a nice to have - but it still only triggers on crashes, and those crashes aren't guaranteed in release builds (where assertions are not enabled). You can see the lower lines in the stack trace show what it looks like in the absence of debug info - you'll still get demangled symbol names, etc.

I think it's really important that we're on the same page about error handling before flang is committed to the LLVM repository.

1) assert+llvm_unreachable for invariant violations 2) llvm::Error (or other error handling mechanisms - clang's Diagnostics system, true/false/errorcode returns sometimes work just fine, etc) for things reachable by valid user behavior (including invalid code, network failures, what-have-you) 3) report_fatal_error - awkward stop-gap when someone hasn't plumbed through an appropriate library-friendly error handling interface (every use should be considered an implicit FIXME)

This has been an ongoing source of tension with the lldb project, for instance, which I'd like to avoid repeating.

I don't know whether @tskeith represents a major position in the f18/flang project, whether his concerns were addressed by the call you're referring to, @DavidTruby - but that perspective sounds in conflict with my own in this regard & still seems outstanding.

Note that even with LLVM's general crash reporting improvements shown above, or Clang's full reproducer feature - these won't enable invariant checking in release builds. Assertions aren't enabled, UB is invoked, and sometimes that UB leads to a crash - with a stack trace (and reproduction steps, in Clang's case). I don't know if that sufficiently addresses the f18/flang/ @tskeith's concerns - if not, we're still at odds.

[sscalpone]

@dwblaikie Thank you for the tour of the existing mechanism. Very helpful & not something that I've looked into yet besides a quick reading of the LLVM Programmer's Manual.

Is there a recommended way to implement checks for invariant violations in an RELEASE build?

If there is a ban on invariant checking of any kind in a release build, is this ban by fiat or related to an undesirable characteristic, say compile-time performance overhead? Would an invariant check be allowed if the resulting code ran faster?

[joker-eph]

On our bi-weekly call earlier, @sscalpone agreed with my above comment as a way forward. @dwblaikie and @joker-eph are you ok with us going ahead with the merge tomorrow on these terms?

My only concern at the moment is a meta-question that is raised by thread like this one, is my perception of the attitude displayed by @tskeith as fairly opposed to accepting to look at the system as a whole and trying to carve flang into an island with their own personal preference. I'm not saying it is a blocker to merge flang, but I can definitely anticipate some frictions down the road.

Is there a recommended way to implement checks for invariant violations in an RELEASE build?

Yes: -DLLVM_ENABLE_ASSERTIONS=ON ;) Another answer is that there is no recommended way because this is not something that is considered a good practice in the LLVM project. The overall approach IMO is that assert() is the only tool to check for invariant, if you want them in your release build then you can ship with assertions enabled, no one will prevent you from doing this.

There aren't any assertions in LLVM that is "expected" to fail in this sense. There is no distinction between debug_assert and release_assert because no one has come up with a principled way to describe what the difference would be and principled rule to pick one or the other, so there is just "assert". If you find a good way to justify this distinction and formulate it, it is worth discussing with the llvm community as whole. It should apply equally to every project though.

In the past I phrased the property I expect from libraries like this: "we should be able to plug a Fuzzer to any public API and not trigger any assertions or it is a bug to be fixed".

[dwblaikie]

On our bi-weekly call earlier, @sscalpone agreed with my above comment as a way forward. @dwblaikie and @joker-eph are you ok with us going ahead with the merge tomorrow on these terms?

My only concern at the moment is a meta-question that is raised by thread like this one, is my perception of the attitude displayed by @tskeith as fairly opposed to accepting to look at the system as a whole and trying to carve flang into an island with their own personal preference. I'm not saying it is a blocker to merge flang, but I can definitely anticipate some frictions down the road.

Is there a recommended way to implement checks for invariant violations in an RELEASE build?

Yes: -DLLVM_ENABLE_ASSERTIONS=ON ;) Another answer is that there is no recommended way because this is not something that is considered a good practice in the LLVM project. The overall approach IMO is that assert() is the only tool to check for invariant, if you want them in your release build then you can ship with assertions enabled, no one will prevent you from doing this.

There aren't any assertions in LLVM that is "expected" to fail in this sense. There is no distinction between debug_assert and release_assert because no one has come up with a principled way to describe what the difference would be and principled rule to pick one or the other, so there is just "assert". If you find a good way to justify this distinction and formulate it, it is worth discussing with the llvm community as whole. It should apply equally to every project though.

Generally agree to all that ^

In the past I phrased the property I expect from libraries like this: "we should be able to plug a Fuzzer to any public API and not trigger any assertions or it is a bug to be fixed".

Hmm, that seems quite different from my expectation. For the command line utilities, yeah, I'd expect to be able to fuzz the input files and command line without bounds. For an API - unbounded fuzzing doesn't seem appropriate, as APIs generally have contracts that need to be adhered to and aren't suitable for runtime error reporting (perhaps this is consistent with what you mean when you say "plug a fuzzer in") - such as certain parameters must not be null (a fuzzer could be constrained to never pass in null), maybe that certain inputs should be well formed (while providing an API to test for well-formedness) or must only be the result of some other function call (you can't create your own magic handle value - you can only use the handles that came from the matching handle creation API, etc)

[joker-eph]

as APIs generally have contracts that need to be adhered to and aren't suitable for runtime error reporting (perhaps this is consistent with what you mean when you say "plug a fuzzer in") - such as certain parameters must not be null (a fuzzer could be constrained to never pass in null),

Yes this is what I mean, there are obvious limitation to "fuzzing". To take a more practical LLVM/MLIR example: any IR should either fail the verifier or not trigger an assertion in a pass. You should be able to "fuzz" passes with any random IR with such principle. Here the public API for a pass is "any valid IR" and the verifier is the way to check it in a release build of the compiler.

[sscalpone]

Yes: -DLLVM_ENABLE_ASSERTIONS=ON ;)

Enabling all debug code is not the desire.

For example, non-trivial NDEBUG code exists. E.g. /lib/CodeGen/GlobalISel/InstructionSelect.cpp.

Also, NDEBUG cannot generally be applied selectively to projects because some classes differ with/without NDEBUG, e.g. class LPMUpdater seems to be different with and without NDEBUG; it has an extra data member.

There is no distinction between debug_assert and release_assert because no one has come up with a principled way to describe what the difference would be and principled rule to pick one or the other, so there is just "assert". If you find a good way to justify this distinction and formulate it, it is worth discussing with the llvm community as whole. It should apply equally to every project though.

I'll give it a shot.

Is this use of report_fatal_error, from llvm/lib/Analysis/TypeBasedAliasAnalysis.cpp, acceptable?

static const MDNode *getLeastCommonType(const MDNode *A, const MDNode *B) {
  if (!A || !B)
    return nullptr;

  if (A == B)
    return A;

  SmallSetVector<const MDNode *, 4> PathA;
  TBAANode TA(A);
  while (TA.getNode()) {
    if (PathA.count(TA.getNode()))
      report_fatal_error("Cycle found in TBAA metadata.");
    PathA.insert(TA.getNode());
    TA = TA.getParent();
  }

[dwblaikie]

In general - pretty much every report_fatal_error is a stop-gap.

That one in TypeBasedAliasAnalysis looks like it should be an assert, but I don't know too much about it - as Mehdi mentioned, the LLVM IR verifier should be used to enforce constraints like this (not sure what it's rules are no metadata - perhaps all metadata is considered verified & it's up to things that consume it to ignore it if it's ill-formed)