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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
