[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D135551#3850308 <https://reviews.llvm.org/D135551#3850308>, @dblaikie wrote:

> In D135551#3850266 <https://reviews.llvm.org/D135551#3850266>, @inclyc wrote:
>
>> This makes sense! However I think `assert(0)` should not be used in this case, we could expose another `llvm_unreachable`-like api and probably `llvm_report_error` shall be fine. Are there some changed assertions actually "Aspirationally unreachable" in this patch?
>
> No, I really don't think  we should go down that path.
>
> I believe these are not actually distinct cases - in either case, the program has UB if they violated the invariants/preconditions - whether or not they called through the C API.

The C Index test cases I commented on earlier in the review are a good example of when there's no UB but we still want to alert people to the problem of code they should not be reaching. The assumption that "reached here unexpectedly" == "UB" is invalid. Some things are logic bugs that exhibit no UB.

> unreachable is no more a guarantee/proven thing than an assertion - both are written by humans and a claim "if this is reached-or-false, there is a bug in some code, somewhere". The statement is not stronger in the unreachable case and the style guide supports that perspective and the way we triage/treat bugs is pretty consistent with that - we get bugs all the time when an unreachable is reached and that doesn't seem to surprise most/anyone - we treat it the same as a bug when an assertion fires.
>
> The discourse discussion, I think, supports this ^ perspective.
>
> As there's still disagreement, should this escalate to the RFC process to change the style guide, Aaron?

Yes, I would appreciate that. I don't think we're interpreting our policy the same way. Specifically "Use llvm_unreachable to mark a specific point in code that should never be reached." -- "should" is turning out to be interpreted in two ways:

- "used to indicate obligation, duty, or correctness, typically when criticizing someone's actions. e.g., he should have been careful": I am asserting it is impossible to reach this.
- "used to indicate what is probable. e.g., $348 million should be enough to buy him out": I am asserting you probably won't get here, but you won't be happy if you do.

In D135551#3850266 <https://reviews.llvm.org/D135551#3850266>, @inclyc wrote:

> This makes sense! However I think `assert(0)` should not be used in this case, we could expose another `llvm_unreachable`-like api and probably `llvm_report_error` shall be fine. Are there some changed assertions actually "Aspirationally unreachable" in this patch?

I'm totally fine not using `assert(0)` and using an `llvm_unreachable`-like API (or even using a macro to dispatch to `llvm_unreachable` under a different name).

There are more aspirationally unreachable issues in this patch, I've commented on the ones I spotted, but I stopped commenting pretty quickly because I think a lot of the cases are made slightly worse by switching to `llvm_unreachable` instead of more targeted changes. I'd be especially curious to hear what @dblaikie thinks of the suggestions I have though -- it might be easier to see the distinction with real world code (or it might not!).



================
Comment at: clang/include/clang/AST/Redeclarable.h:265
         if (PassedFirst) {
-          assert(0 && "Passed first decl twice, invalid redecl chain!");
+          llvm_unreachable("Passed first decl twice, invalid redecl chain!");
           Current = nullptr;
----------------
This looks like it should probably be: `assert(!PassedFirst && "Passed first decl twice, invalid redecl chain!");` rather than an `if` statement with recovery mechanisms.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:601
     assert(isa<ObjCMethodDecl>(FD));
-    assert(false && "Getting the type of ObjCMethod is not supported yet");
+    llvm_unreachable("Getting the type of ObjCMethod is not supported yet");
 
----------------
This looks like it possibly should have been an error reported to the user? If not, this is a wonderful example of what I mean by aspirationally unreachable code. We aspire to not getting here -- but we can get here just the same and there is not UB as a result (we return a valid object, UB might happen elsewhere based on invalid assumptions of what is returned).


================
Comment at: clang/lib/AST/ASTImporter.cpp:9976
   else {
-    assert(0 && "CompleteDecl called on a Decl that can't be completed");
+    llvm_unreachable("CompleteDecl called on a Decl that can't be completed");
   }
----------------
According to our style guide, this probably should have been `assert(isa<ObjCInterfaceDecl, ObjCProtocolDecl, TagDecl>(D) && "CompleteDecl called on a Decl that can't be completed");` but IMO that's worse than using `assert(0)` because it's less maintainable (any time you add a new else if chain you have to also update the assert). I think `llvm_unreachable` is wrong to use here.


================
Comment at: clang/lib/AST/ExprConstant.cpp:7561-7564
     if (Source == E) {
-      assert(0 && "OpaqueValueExpr recursively refers to itself");
+      llvm_unreachable("OpaqueValueExpr recursively refers to itself");
       return Error(E);
     }
----------------
Probably should be `assert(Source != E && "OpaqueValueExpr recursively refers to itself");`


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:136-137
 
   default:
-    assert(false && "Cast not implemented");
+    llvm_unreachable("Cast not implemented");
   }
----------------
I think this can just be removed, right? There's another unreachable for falling out of the `switch` that seems to be covering the same situation.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:598
       } else {
-        assert(false && "Unhandled type in array initializer initlist");
+        llvm_unreachable("Unhandled type in array initializer initlist");
       }
----------------
The rest of the ones here are somewhat interesting in that the interpreter is an experiment under active development and is known to be incomplete. In all of these cases, I think the switch to unreachable is flat-out wrong -- these asserts serve explicitly to find unimplemented cases when we hit them.


================
Comment at: clang/lib/Basic/SourceManager.cpp:62-65
   if (Buffer == nullptr) {
-    assert(0 && "Buffer should never be null");
+    llvm_unreachable("Buffer should never be null");
     return llvm::MemoryBuffer::MemoryBuffer_Malloc;
   }
----------------
`assert(Buffer != nullptr && "Buffer should never be null");`

but that said, this one might be an optimization hint that suggests we should be using `__builtin_assume(Buffer != nullptr)`, I'm not certain.


================
Comment at: clang/lib/Basic/SourceManager.cpp:863-866
   if (SLocOffset < CurrentLoadedOffset) {
-    assert(0 && "Invalid SLocOffset or bad function choice");
+    llvm_unreachable("Invalid SLocOffset or bad function choice");
     return FileID();
   }
----------------
`assert(SLocOffset >= CurrentLoadedOffset && "Invalid SLocOffset or bad function choice");`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135551/new/

https://reviews.llvm.org/D135551