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

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

dblaikie:

> In the same way that an assert says "This condition should never be false" - 
> I use "should" in the same sense in both unreachable and assert, and I 
> believe that's the prevailing opinion of LLVM developers/the LLVM style guide.

aaronballman:

> I believe our code base says something different than our "prevailing 
> opinion" and we should not be discounting the reality of how our code is 
> written today.

So, there are lots of ways and places where "the reality of how our code is 
written today" fails to conform to the style guide.
One conclusion is that the style guide is wrong and should be changed to 
reflect reality.
Another conclusion is that conforming to the style guide is the goal and one 
aspect of development is to strive to reach the goal. The imperfection of the 
state of the code today merely reflects an imperfect understanding of the goal.

I suggest a Round Table at the (less than one month away) Dev Meeting, assuming 
that the active parties in this discussion will be there. Put off any RFC until 
after that.


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

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

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

In D135551#3853365 , @rnk wrote:

> I think the status quo has real problems. We pretend that we can do both of 
> these:
>
> - Assert liberally, with the understanding that assertion failures lead to UB 
> (failed bad cast check, bounds checks, unreachable code, etc)
> - We can actually find and fix all cases that violate those inputs to the 
> point that clang is stable and secure enough for our satisfaction
>
> Currently, it is really easy to run fuzzers and find crash bugs in clang. I 
> think the lesson we should take from that is that we are compromising goal 2 
> here, and we shouldn't kid ourselves about it.
>
> Maybe the goal is not security, but is instead something about user or 
> developer experience, but we should go through some higher level process to 
> clarify that goal so we can write it down and agree on it.

+1 to all of this


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

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

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.



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");
   }

aaron.ballman wrote:
> dblaikie wrote:
> > aaron.ballman wrote:
> > > 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.
> > & I don't see why unreachable is any different a statement than 
> > assert(false) in these cases... - it's the same statement of intent. "if 
> > this is reached you've found a bug" (in this case, a missing feature)
> > 
> > But I'd be sort of OK changing all these to report_fatal_error. But, again, 
> > I think the assert(false) -> unreachable is a valid transformation and 
> > doesn't make anything worse than it already is, but improves the code by 
> > being more consistent and removing this confusion that there might be 
> > something different about assert(false) when, I believe, there isn't.
> > & I don't see why unreachable is any different a statement than 
> > assert(false) in these cases... - it's the same statement of intent. "if 
> > this is reached you've found a bug" (in this case, a missing feature)
> 
> You are asserting it's the same statement of intent and I keep pointing out 
> that people use the different constructs in practice because they're 
> different statements of intent. I don't know how to resolve this difference 
> of opinion, but I can say as someone doing code review in this area recently 
> that your interpretation is wrong according to what we were after with this 
> code.
> 
> I'd be fine changing it to `report_fatal_error` instead of `assert(false)`; 
> I'd be strongly opposed to switching to `llvm_unreachable`.
I use llvm_unreachable as a nicer to use assert in if/else chains like this. I 
also see no difference in the intent between assert and unreachable; assert(0 
&& "message") is just uglier.

report_fatal_error is for something a user could plausibly run into but also 
isn't worth wiring into a proper error diagnostic (which happens a lot in 
codegen)


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

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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I think the disagreement here highlights the need to have a serious discussion 
about the future of error handling across the LLVM project. As you say, it 
sounds like you're not going to reach agreement on this code review, so maybe 
the best short term next step is to land the uncontroversial changes that Aaron 
agrees with.

Regarding error handling and the wide usage of assertions to guard UB across 
LLVM, we need to decide what our goals are as a community. Is it actually the 
goal of the project that Clang and LLVM that no input can lead to UB? If we 
can't guarantee that, is there some error budget we consider acceptable (fuzzer 
runs for 24hrs and can't find bugs)? How does that goal rate against our other 
goals, like performance? We could just ship with assertions enabled, sacrifice 
20% code size and performance, and call it a day. We used to do that for 
Chromium, but users complained that compiles were too slow and we stopped doing 
it.

I think the status quo has real problems. We pretend that we can do both of 
these:

- Assert liberally, with the understanding that assertion failures lead to UB 
(failed bad cast check, bounds checks, unreachable code, etc)
- We can actually find and fix all cases that violate those inputs to the point 
that clang is stable and secure enough for our satisfaction

Currently, it is really easy to run fuzzers and find crash bugs in clang. I 
think the lesson we should take from that is that we are compromising goal 2 
here, and we shouldn't kid ourselves about it.

Maybe the goal is not security, but is instead something about user or 
developer experience, but we should go through some higher level process to 
clarify that goal so we can write it down and agree on it.




Comment at: clang/lib/Analysis/CFG.cpp:1047
+  llvm_unreachable("Unexpected unary operator!");
   return llvm::None;
 }

This will create unreachable code warnings, which must be addressed before 
landing.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D135551#3850511 , @dblaikie wrote:

> In D135551#3850391 , @aaron.ballman 
> wrote:
>
>> In D135551#3850308 , @dblaikie 
>> wrote:
>>
>>> In 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."
>
> In the same way that an assert says "This condition should never be false" - 
> I use "should" in the same sense in both unreachable and assert, and I 
> believe that's the prevailing opinion of LLVM developers/the LLVM style guide.

I believe our code base says something different than our "prevailing opinion" 
and we should not be discounting the reality of how our code is written today.

> Perhaps we are also at a deadlock as to who should write the proposal... :/

Agreed, you and I are probably both too close to the issue to write the 
proposal right now. If nobody else does it first, maybe the two of us could 
circle back in a few months after we've had time to research and think more 
deeply and we could co-write something (even if it is a multiple choice RFC 
between conflicting directions).

>> - "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.
>
> This is the "should" ^ I mean, and what every assert should mean too. This 
> code assumes this property to be true - this is a precondition of the code.
>
> We should not be using asserts where we don't mean this. I'm OK assuming 
> every assert does mean "this is a precondition" and treating them that way in 
> terms of transforming them to unreachable or anything else we might do with 
> them - and if some of them don't mean it, then they're buggy and we can fix 
> them, but assert->unreachable doesn't make the situation any worse.
>
> Any code behind/after an assert is untested and unvalidated - we can't say 
> "if you violate this constraint it'll actually be OK" because we've never 
> tested that/don't know that.

Just to double-check... are you opposed to the idea of differentiating between 
ways of saying "the reachability of this code is in question" or are you 
opposed to use of `assert` (or something that smells too similar) specifically? 
Because I don't care about *how* we spell the differentiation, just that 
there's not a policy limiting my ability to express my intent in code. I'd be 
perfectly fine with `#define some_name_we_agree_upon(msg) 
llvm_unreachable(msg)` being the facility we use instead of `assert(0);`.

>> - "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.

I'm arguing that we have a not-insignificant amount of uses that have this^ 
interpretation and I want a way to express that distinction in code. I have an 
allergic reaction to using `llvm_unreachable` to express 
known-to-be-potentially reachable code because my complaint is that the 
annotation c

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a subscriber: rnk.
dblaikie added a comment.

In D135551#3850391 , @aaron.ballman 
wrote:

> In D135551#3850308 , @dblaikie 
> wrote:
>
>> In 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."

In the same way that an assert says "This condition should never be false" - I 
use "should" in the same sense in both unreachable and assert, and I believe 
that's the prevailing opinion of LLVM developers/the LLVM style guide.

Perhaps we are also at a deadlock as to who should write the proposal... :/

> - "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.

This is the "should" ^ I mean, and what every assert should mean too. This code 
assumes this property to be true - this is a precondition of the code.

We should not be using asserts where we don't mean this. I'm OK assuming every 
assert does mean "this is a precondition" and treating them that way in terms 
of transforming them to unreachable or anything else we might do with them - 
and if some of them don't mean it, then they're buggy and we can fix them, but 
assert->unreachable doesn't make the situation any worse.

Any code behind/after an assert is untested and unvalidated - we can't say "if 
you violate this constraint it'll actually be OK" because we've never tested 
that/don't know that.

> - "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 , @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!).

Maybe - happy to talk about a few of the examples, but I'm not feeling super 
optimistic that we'll come to an understanding here, unfortunately :/

@rnk's comment here ( 
https://discourse.llvm.org/t/llvm-unreachable-is-widely-misused/60587/3 ) 
pretty well sums up my understanding/values here and it looks like on that 
thread, mostly the long term LLVM developers agree with this perspective and 
are trying to explain it to the (so far as I can tell) 

[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 , @dblaikie wrote:

> In 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 , @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(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(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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In 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.

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?


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

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

[YingChi Long via Phabricator via cfe-commits]

inclyc added a comment.

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?


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D135551#3850132 , @inclyc wrote:

> In D135551#3849983 , @aaron.ballman 
> wrote:
>
>> In D135551#3849944 , @inclyc wrote:
>>
 - Prefer llvm_report_error() in any circumstance under which a code path 
 is functionally possible to reach, but only in erroneous executions that 
 signify a mistake on the part of the LLVM developer elsewhere in the 
 program.
 - Prefer llvm_unreachable() in any circumstance under which a code path is 
 believed to be functionally impossible to reach (even if technically 
 possible to reach). The API is now self-documenting to mean "this code 
 really should be totally unreachable".
>>>
>>> I think `llvm_unreachable` already has the functionality reporting bugs for 
>>> developer in our implementation, with +Assertions by default
>>
>> Yes, in terms of its runtime behavior. So long as we're not making debugging 
>> harder for some large group of people, the runtime behavior is not really 
>> what I'm concerned by though. I'm focusing more on code reviewers and 
>> project newcomers and whether our code is self-documenting or not. Having a 
>> policy to use an API that says code is not reachable in situations where 
>> that code is very much reachable is the crux of my problem -- the API is 
>> sometimes a lie (and a lie with optimization impacts, at that) and we force 
>> everyone to pay the cognitive costs associated with that when reading code.
>>
>> If we end up with two APIs that have the same runtime behavior, I'm okay 
>> with that.
>
> Could you elaborate on "aspirationally" vs "functionally" unreachable code 
> here?

Sure!

  enum E { Zero, One, Two };
  
  int func(E Val) {
switch (Val) {
case Zero: return 12;
case One: return 200;
case Two: return -1;
}
llvm_unreachable("never get here"); // Functionally unreachable; we can't 
think of a reasonable way to get here without other alarms going off
  }

vs

  enum E { Zero, One, Two };
  
  extern "C" int func(unsigned Val) {
switch (Val) {
case Zero: return 12;
case One: return 200;
case Two: return -1;
}
assert(0 && "never get here"); // Aspirationally unreachable; we HOPE we 
don't get here but it's plausible that we do if someone made a logical mistake 
calling the API
  }


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

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

[YingChi Long via Phabricator via cfe-commits]

inclyc added a comment.

In D135551#3849983 , @aaron.ballman 
wrote:

> In D135551#3849944 , @inclyc wrote:
>
>>> - Prefer llvm_report_error() in any circumstance under which a code path is 
>>> functionally possible to reach, but only in erroneous executions that 
>>> signify a mistake on the part of the LLVM developer elsewhere in the 
>>> program.
>>> - Prefer llvm_unreachable() in any circumstance under which a code path is 
>>> believed to be functionally impossible to reach (even if technically 
>>> possible to reach). The API is now self-documenting to mean "this code 
>>> really should be totally unreachable".
>>
>> I think `llvm_unreachable` already has the functionality reporting bugs for 
>> developer in our implementation, with +Assertions by default
>
> Yes, in terms of its runtime behavior. So long as we're not making debugging 
> harder for some large group of people, the runtime behavior is not really 
> what I'm concerned by though. I'm focusing more on code reviewers and project 
> newcomers and whether our code is self-documenting or not. Having a policy to 
> use an API that says code is not reachable in situations where that code is 
> very much reachable is the crux of my problem -- the API is sometimes a lie 
> (and a lie with optimization impacts, at that) and we force everyone to pay 
> the cognitive costs associated with that when reading code.
>
> If we end up with two APIs that have the same runtime behavior, I'm okay with 
> that.

Could you elaborate on "aspirationally" vs "functionally" unreachable code here?


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D135551#3849944 , @inclyc wrote:

>> - Prefer llvm_report_error() in any circumstance under which a code path is 
>> functionally possible to reach, but only in erroneous executions that 
>> signify a mistake on the part of the LLVM developer elsewhere in the program.
>> - Prefer llvm_unreachable() in any circumstance under which a code path is 
>> believed to be functionally impossible to reach (even if technically 
>> possible to reach). The API is now self-documenting to mean "this code 
>> really should be totally unreachable".
>
> I think `llvm_unreachable` already has the functionality reporting bugs for 
> developer in our implementation, with +Assertions by default

Yes, in terms of its runtime behavior. So long as we're not making debugging 
harder for some large group of people, the runtime behavior is not really what 
I'm concerned by though. I'm focusing more on code reviewers and project 
newcomers and whether our code is self-documenting or not. Having a policy to 
use an API that says code is not reachable in situations where that code is 
very much reachable is the crux of my problem -- the API is sometimes a lie 
(and a lie with optimization impacts, at that) and we force everyone to pay the 
cognitive costs associated with that when reading code.

If we end up with two APIs that have the same runtime behavior, I'm okay with 
that.


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

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

[YingChi Long via Phabricator via cfe-commits]

inclyc added a comment.

> - Prefer llvm_report_error() in any circumstance under which a code path is 
> functionally possible to reach, but only in erroneous executions that signify 
> a mistake on the part of the LLVM developer elsewhere in the program.
> - Prefer llvm_unreachable() in any circumstance under which a code path is 
> believed to be functionally impossible to reach (even if technically possible 
> to reach). The API is now self-documenting to mean "this code really should 
> be totally unreachable".

I think `llvm_unreachable` already has the functionality reporting bugs for 
developer in our implementation, with +Assertions by default


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D135551#3849816 , @dblaikie wrote:

>>> Generally LLVM's pretty hard to fathom in a non-asserts build anyway, 
>>> right? (that's the first thing any of us do is reproduce with an assertions 
>>> build that may fail miles away from where a crash occurred because an 
>>> invariant was violated much earlier) - that `cast` won't crash/will 
>>> continue on happily in a non-asserts build seems like a much larger hole to 
>>> debuggability of a non-asserts build than any unreachable?
>>
>> This might be true -- personally, I tend to only use debug builds with MSVC 
>> because RelWithDebInfo isn't sufficient for my daily needs. However, I've 
>> definitely heard of folks who use RelWithDebInfo for their daily work 
>> (RelWithDebInfo + Asserts specifically, IIRC) because of the improved build 
>> times and runtime performance; we should be sure we're not disrupting that 
>> workflow too much.
>
> Changing `assert(0)` to `llvm_unreachable` does not change the behavior of 
> any +Asserts build. The behavior of unreachable is the same as assert(0) in a 
> +Asserts build.
>
> Is this observation enough to undeadlock this conversation?

No, it doesn't address the key point I have which is that I want different APIs 
to express intent instead of using `llvm_unreachable` in all cases. To me, it 
is a mistake to label functionally reachable code as being unconditionally 
unreachable. It requires everyone reading that code to understand our nuances 
to know that the API signals aspirationally unreachable code rather than 
functionally unreachable code. These are different scenarios and I don't want 
to lose that distinction in the places where it matters.


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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

>> Generally LLVM's pretty hard to fathom in a non-asserts build anyway, right? 
>> (that's the first thing any of us do is reproduce with an assertions build 
>> that may fail miles away from where a crash occurred because an invariant 
>> was violated much earlier) - that `cast` won't crash/will continue on 
>> happily in a non-asserts build seems like a much larger hole to 
>> debuggability of a non-asserts build than any unreachable?
>
> This might be true -- personally, I tend to only use debug builds with MSVC 
> because RelWithDebInfo isn't sufficient for my daily needs. However, I've 
> definitely heard of folks who use RelWithDebInfo for their daily work 
> (RelWithDebInfo + Asserts specifically, IIRC) because of the improved build 
> times and runtime performance; we should be sure we're not disrupting that 
> workflow too much.

Changing `assert(0)` to `llvm_unreachable` does not change the behavior of any 
+Asserts build. The behavior of unreachable is the same as assert(0) in a 
+Asserts build.

Is this observation enough to undeadlock this conversation?


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D135551#3847962 , @dblaikie wrote:

> In D135551#3847607 , @aaron.ballman 
> wrote:
>
>> In D135551#3847444 , @dblaikie 
>> wrote:
>>
>>> I thought this was settled quite a while ago and enshrined in the style 
>>> guide: https://llvm.org/docs/CodingStandards.html#assert-liberally
>>>
>>> `assert(0)` should not be used if something is reachable. We shouldn't have 
>>> a "this violates an invariant, but if you don't have asserts enabled you do 
>>> get some maybe-acceptable behavior".
>>>
>>> I feel fairly strongly that any cases of "reachable asserts" should be 
>>> changed to valid error handling or `llvm_report_error` and remaining 
>>> `assert(0)` should be transformed to `llvm_unreachable`. (also, ideally, 
>>> don't use branch-to-`unreachable` where possible, instead assert the 
>>> condition - in cases where the `if` has side effects, sometimes that's the 
>>> nicest way to write it, but might be clearer, if more verbose to use a 
>>> local variable for the condition, then assert that the variable is true 
>>> (and have the requisite "variable might be unused" cast))
>>
>> I would be okay with that, but that's not what this patch was doing -- it 
>> was changing `assert(0)` into an `llvm_unreachable` more mechanically, and I 
>> don't think that's a valid transformation. The key, to me, is not losing the 
>> distinction between "reaching here is a programming mistake that you'd make 
>> during active development" vs "we never expect to reach this patch and want 
>> to optimize accordingly."
>
> I don't really think those are different things, though. Violating invariants 
> is UB and there's no discussion to be had about how the program (in a 
> non-asserts build) behaves when those invariants are violated - all bets are 
> off, whether it's assert or unreachable.

I think they're the same thing in terms of runtime behavior, but I feel (rather 
strongly) that they're different in terms of documentation when reading the 
source. This code pattern exists and keeps coming up year after year, which is 
sufficient to inform me that the community thinks there is *some* distinction 
to be made there. Also, the fact that we have report_fatal_error *and* 
unreachable APIs signals that we already understand there's a distinction 
between "reaching here will never happen; optimize accordingly" and "reaching 
here is a surprising mistake".

>> `__builtin_unreachable` changes the debugging landscape far too much for me 
>> to want to see folks using it for "reaching here is a programming mistake" 
>> situations, *especially* in RelWithDebInfo builds where optimizations are 
>> enabled and may result in surprising call stacks and time travel debugging.
>
> Generally LLVM's pretty hard to fathom in a non-asserts build anyway, right? 
> (that's the first thing any of us do is reproduce with an assertions build 
> that may fail miles away from where a crash occurred because an invariant was 
> violated much earlier) - that `cast` won't crash/will continue on happily in 
> a non-asserts build seems like a much larger hole to debuggability of a 
> non-asserts build than any unreachable?

This might be true -- personally, I tend to only use debug builds with MSVC 
because RelWithDebInfo isn't sufficient for my daily needs. However, I've 
definitely heard of folks who use RelWithDebInfo for their daily work 
(RelWithDebInfo + Asserts specifically, IIRC) because of the improved build 
times and runtime performance; we should be sure we're not disrupting that 
workflow too much.

 Historically, we've usually used llvm_unreachable for situations where 
 we're saying "this code cannot be reached; if it can, something else has 
 gone seriously wrong." For example, in code like: int foo(SomeEnum E) { 
 switch (E) { case One: return 1; default: return 2; } 
 llvm_unreachable("getting here would be mysterious"); } and we've used 
 assert(0) for situations where we're saying "this code is possible to 
 reach only when there were mistakes elsewhere which broke invariants we 
 were relying on."
>>>
>>> I don't think those are different things though - violating invariants is 
>>> ~= something going seriously wrong.
>>>
>>> (sorry, I guess I should debate this on the thread instead of here - but I 
>>> think most of the folks on that thread did agree with the LLVM style 
>>> guide/the direction here)
>>>
>>> This, I think was also discussed about a decade ago in the LLVM community 
>>> and resulted in r166821/2962d9599e463265edae599285bbc6351f1cc0ef which 
>>> specifically "Suggests llvm_unreachable over assert(0)" and is the policy 
>>> of LLVM - this change is consistent with that policy.
>>
>> I don't have the context for those changes in my email either, but 
>> regardless of what we thought ten years ago, we have code in the code base 
>>

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Because I was looking, some other `assert(0)` -> `llvm_unreachable` cleanups 
(though, yes, even the earliest cleanups include some assert(0) -> 
report_fatal_error, but for externally/user-reachable failures, like invalid 
bitcode, I think). Some of these are more blanket/wide reaching than others, 
for sure. (& no doubt the naive search through the commit log doesn't find all 
the cleanups)

I /think/ when I was looking I did find one from 2017 that Richard Smith 
approved that included llvm_unreachable in the Clang C API, in terms of more 
recent precedent for some of this... but can't find that again right now.

rGabd1561f15e 
:[LLDBAssert]
 Use unreachable instead of assert(0)
rGf9da10ce4544ea66fe6fad5b943d3700d192a1e1 
:Change to 
assert(0,x) to llvm_unreachable(x)
rG7a247f709baa2cd19111af9e18965df4e419949a 
:Turn 
effective assert(0) into llvm_unreachable
rG35b2f75733c98e5904c5a75f8bcedeb96c4f4eda 
:Convert 
some assert(0) to llvm_unreachable or fold an 'if' condition into the assert.
rGd8d43191d8afe2c4b5b2d3be62cd73ddc3ddc6c9 
:Replace 
some assert(0)'s with llvm_unreachable.
rG2a30d7889fc54c8a74d73b79be3dd030bac41b06 
:Replace 
some assert(0)'s with llvm_unreachable.
rG0039f3f0607702f2d16d60addff74c67869e2144 
:Replace 
some assert(0)'s with llvm_unreachable.
rGc7193c48d9d7e44e9fd0c39205e8b7cfbf5d5458 
:Convert 
assert(0) to llvm_unreachable to silence a warning about Addend being 
uninitialized in default case.
rG7b7a67c5c8daa051e42837dc3e5e65adab9cf09c 
:[ARM64] 
Fix 'assert("...")' to be 'assert(0 && "...")'. Otherwise, it is no assert at 
all. ;] Some of these should probably be switched to llvm_unreachable, but I 
didn't want to perturb the behavior in this patch.
rGeaa3a7efab65d0a65ddda7fdb7e5fbdbd5f897ad 
:Use 
llvm_unreachable instead of assert(0)
rGd7fd95a5c1eb19e5754281b540f09e86ced1b9d4 
:Change 
assert(0 && "text") to llvm_unreachable(0 && "text")
rG2962d9599e463265edae599285bbc6351f1cc0ef 
:Suggest 
llvm_unreachable over assert(0).
rG2e007de42de48dc05bdb7aa9d3c9e8902ee720fe 
:Revert 
"Target/AMDGPU/AMDILIntrinsicInfo.cpp: Use llvm_unreachable() in nonreturn 
function, instead of assert(0)."
rGdc4261794fdbf2e3001f369d8b8bbd77eb923602 
:Target/AMDGPU/AMDILIntrinsicInfo.cpp:
 Use llvm_unreachable() in nonreturn function, instead of assert(0).
rG751eb3d2b30d90069b1797a31f8e489f0763c585 
:use 
llvm_unreachable() instead of assert(0) for invalid enum values in switch 
statements
rGbdf39a46a302df7cb07e948a6780265b3335fbda 
:Convert 
assert(0) to llvm_unreachable.
rGeb455832b4c7cb1046ab9fb9f6f8ca40202874a4 
:Silence 
various build warnings from Hexagon backend that show up in release builds. 
Mostly converting 'assert(0)' to 'llvm_unreachable' to silence warnings about 
missing returns. Also fold some variable declarations into asserts to prevent 
the variables from being unused in release builds.
rGabd1561f15ee466c0fd9abeede2cdcde2ebb2cec 
:[LLDBAssert]
 Use unreachable instead of assert(0)
rGf9da10ce4544ea66fe6fad5b943d3700d192a1e1 
:Change to 
assert(0,x) to llvm_unreachable(x)
rG7a247f709baa2cd19111af9e18965df4e419949a 
:Turn 
effective assert(0) into llvm_unreachable
rG35b2f75733c98e5904c5a75f8bcedeb96c4f4eda 
:Convert 
some assert(0) to llvm_unreachable or fold an 'if' condition into the assert.
rGd8d43191d8afe2c4b5b2d3be62cd73ddc3ddc6c9 
:Replace 
some assert(0)'s with llvm_unreachable.
rG2a30d7889fc54c8a74d73b79be3dd030bac41b06 

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D135551#3847607 , @aaron.ballman 
wrote:

> In D135551#3847444 , @dblaikie 
> wrote:
>
>> I thought this was settled quite a while ago and enshrined in the style 
>> guide: https://llvm.org/docs/CodingStandards.html#assert-liberally
>>
>> `assert(0)` should not be used if something is reachable. We shouldn't have 
>> a "this violates an invariant, but if you don't have asserts enabled you do 
>> get some maybe-acceptable behavior".
>>
>> I feel fairly strongly that any cases of "reachable asserts" should be 
>> changed to valid error handling or `llvm_report_error` and remaining 
>> `assert(0)` should be transformed to `llvm_unreachable`. (also, ideally, 
>> don't use branch-to-`unreachable` where possible, instead assert the 
>> condition - in cases where the `if` has side effects, sometimes that's the 
>> nicest way to write it, but might be clearer, if more verbose to use a local 
>> variable for the condition, then assert that the variable is true (and have 
>> the requisite "variable might be unused" cast))
>
> I would be okay with that, but that's not what this patch was doing -- it was 
> changing `assert(0)` into an `llvm_unreachable` more mechanically, and I 
> don't think that's a valid transformation. The key, to me, is not losing the 
> distinction between "reaching here is a programming mistake that you'd make 
> during active development" vs "we never expect to reach this patch and want 
> to optimize accordingly."

I don't really think those are different things, though. Violating invariants 
is UB and there's no discussion to be had about how the program (in a 
non-asserts build) behaves when those invariants are violated - all bets are 
off, whether it's assert or unreachable.

> `__builtin_unreachable` changes the debugging landscape far too much for me 
> to want to see folks using it for "reaching here is a programming mistake" 
> situations, *especially* in RelWithDebInfo builds where optimizations are 
> enabled and may result in surprising call stacks and time travel debugging.

Generally LLVM's pretty hard to fathom in a non-asserts build anyway, right? 
(that's the first thing any of us do is reproduce with an assertions build that 
may fail miles away from where a crash occurred because an invariant was 
violated much earlier) - that `cast` won't crash/will continue on happily in a 
non-asserts build seems like a much larger hole to debuggability of a 
non-asserts build than any unreachable?

>>> Historically, we've usually used llvm_unreachable for situations where 
>>> we're saying "this code cannot be reached; if it can, something else has 
>>> gone seriously wrong." For example, in code like: int foo(SomeEnum E) { 
>>> switch (E) { case One: return 1; default: return 2; } 
>>> llvm_unreachable("getting here would be mysterious"); } and we've used 
>>> assert(0) for situations where we're saying "this code is possible to reach 
>>> only when there were mistakes elsewhere which broke invariants we were 
>>> relying on."
>>
>> I don't think those are different things though - violating invariants is ~= 
>> something going seriously wrong.
>>
>> (sorry, I guess I should debate this on the thread instead of here - but I 
>> think most of the folks on that thread did agree with the LLVM style 
>> guide/the direction here)
>>
>> This, I think was also discussed about a decade ago in the LLVM community 
>> and resulted in r166821/2962d9599e463265edae599285bbc6351f1cc0ef which 
>> specifically "Suggests llvm_unreachable over assert(0)" and is the policy of 
>> LLVM - this change is consistent with that policy.
>
> I don't have the context for those changes in my email either, but regardless 
> of what we thought ten years ago, we have code in the code base today that 
> assumes a difference in severity between kinds of unreachable statements so 
> we need to be careful when correcting mistakes. I think we're still in 
> agreement that `llvm_unreachable` should be preferred over `assert(0)` in 
> situations where the code is expected to be impossible to reach. I think 
> we're also still in agreement that "correct error reporting" is preferred 
> over `assert(0)`. Where we might still have daylight are the occasional 
> situations where `assert(0)` gives a better experience -- when the code is 
> possible to reach but reaching it signifies a developer (not user) mistake 
> that is plausible to make when doing new development, such as when misusing 
> the C interface (where there isn't always an error that should be reported 
> via the API). I think these uses should generally be rare, but I don't think 
> it's a "never do this" kind of situation.

That still seems like something that should be caught by an asserts build and 
is UB otherwise. If you're developing against LLVM's APIs in a non-assertions 
build, there's a lot of other invariants that won't be checked (`cast` be

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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

I would think we could convert every assert(0) to either 
`llvm::report_fatal_error` (guaranteed trap) or `llvm_unreachable()` (trap or 
optimize, depending on CMAKE configuration). The C API usage checks seem like 
good candidates for the former.

Also, not sure if everyone noticed, but the latter can now be configured to 
always trap by turning off the “optimize” CMAKE flag. This seems useful for 
fuzzing situations where you may not want asserts builds.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D135551#3847444 , @dblaikie wrote:

> I thought this was settled quite a while ago and enshrined in the style 
> guide: https://llvm.org/docs/CodingStandards.html#assert-liberally
>
> `assert(0)` should not be used if something is reachable. We shouldn't have a 
> "this violates an invariant, but if you don't have asserts enabled you do get 
> some maybe-acceptable behavior".
>
> I feel fairly strongly that any cases of "reachable asserts" should be 
> changed to valid error handling or `llvm_report_error` and remaining 
> `assert(0)` should be transformed to `llvm_unreachable`. (also, ideally, 
> don't use branch-to-`unreachable` where possible, instead assert the 
> condition - in cases where the `if` has side effects, sometimes that's the 
> nicest way to write it, but might be clearer, if more verbose to use a local 
> variable for the condition, then assert that the variable is true (and have 
> the requisite "variable might be unused" cast))

I would be okay with that, but that's not what this patch was doing -- it was 
changing `assert(0)` into an `llvm_unreachable` more mechanically, and I don't 
think that's a valid transformation. The key, to me, is not losing the 
distinction between "reaching here is a programming mistake that you'd make 
during active development" vs "we never expect to reach this patch and want to 
optimize accordingly." `__builtin_unreachable` changes the debugging landscape 
far too much for me to want to see folks using it for "reaching here is a 
programming mistake" situations, *especially* in RelWithDebInfo builds where 
optimizations are enabled and may result in surprising call stacks and time 
travel debugging.

>> Historically, we've usually used llvm_unreachable for situations where we're 
>> saying "this code cannot be reached; if it can, something else has gone 
>> seriously wrong." For example, in code like: int foo(SomeEnum E) { switch 
>> (E) { case One: return 1; default: return 2; } llvm_unreachable("getting 
>> here would be mysterious"); } and we've used assert(0) for situations where 
>> we're saying "this code is possible to reach only when there were mistakes 
>> elsewhere which broke invariants we were relying on."
>
> I don't think those are different things though - violating invariants is ~= 
> something going seriously wrong.
>
> (sorry, I guess I should debate this on the thread instead of here - but I 
> think most of the folks on that thread did agree with the LLVM style 
> guide/the direction here)
>
> This, I think was also discussed about a decade ago in the LLVM community and 
> resulted in r166821/2962d9599e463265edae599285bbc6351f1cc0ef which 
> specifically "Suggests llvm_unreachable over assert(0)" and is the policy of 
> LLVM - this change is consistent with that policy.

I don't have the context for those changes in my email either, but regardless 
of what we thought ten years ago, we have code in the code base today that 
assumes a difference in severity between kinds of unreachable statements so we 
need to be careful when correcting mistakes. I think we're still in agreement 
that `llvm_unreachable` should be preferred over `assert(0)` in situations 
where the code is expected to be impossible to reach. I think we're also still 
in agreement that "correct error reporting" is preferred over `assert(0)`. 
Where we might still have daylight are the occasional situations where 
`assert(0)` gives a better experience -- when the code is possible to reach but 
reaching it signifies a developer (not user) mistake that is plausible to make 
when doing new development, such as when misusing the C interface (where there 
isn't always an error that should be reported via the API). I think these uses 
should generally be rare, but I don't think it's a "never do this" kind of 
situation.

`llvm_unreachable` asserts in debug mode, so it has the nice property we need 
when doing new development. But it doesn't convey the distinction in semantics. 
Maybe another approach is: `#define developer_bugcheck(msg) 
llvm_unreachable(msg)` (or something along those lines). We still get the 
assertion in debug mode, we then get the better optimization in release mode, 
but we don't lose the semantic information in the code base. It doesn't really 
help the RelWithDebInfo case though (where call stacks may be unrecognizable as 
a result of time travel optimizations) but maybe that's a tradeoff worth making?


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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Can't seem to find an llvm-dev/commits discussion for r166821, but I remember 
discussing it several times before, perhaps this one happened on IRC and so we 
may not have any record of it.


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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

I thought this was settled quite a while ago and enshrined in the style guide: 
https://llvm.org/docs/CodingStandards.html#assert-liberally

`assert(0)` should not be used if something is reachable. We shouldn't have a 
"this violates an invariant, but if you don't have asserts enabled you do get 
some maybe-acceptable behavior".

I feel fairly strongly that any cases of "reachable asserts" should be changed 
to valid error handling or `llvm_report_error` and remaining `assert(0)` should 
be transformed to `llvm_unreachable`. (also, ideally, don't use 
branch-to-`unreachable` where possible, instead assert the condition - in cases 
where the `if` has side effects, sometimes that's the nicest way to write it, 
but might be clearer, if more verbose to use a local variable for the 
condition, then assert that the variable is true (and have the requisite 
"variable might be unused" cast))

> Historically, we've usually used llvm_unreachable for situations where we're 
> saying "this code cannot be reached; if it can, something else has gone 
> seriously wrong." For example, in code like: int foo(SomeEnum E) { switch (E) 
> { case One: return 1; default: return 2; } llvm_unreachable("getting here 
> would be mysterious"); } and we've used assert(0) for situations where we're 
> saying "this code is possible to reach only when there were mistakes 
> elsewhere which broke invariants we were relying on."

I don't think those are different things though - violating invariants is ~= 
something going seriously wrong.

(sorry, I guess I should debate this on the thread instead of here - but I 
think most of the folks on that thread did agree with the LLVM style guide/the 
direction here)

This, I think was also discussed about a decade ago in the LLVM community and 
resulted in r166821/2962d9599e463265edae599285bbc6351f1cc0ef which specifically 
"Suggests llvm_unreachable over assert(0)" and is the policy of LLVM - this 
change is consistent with that policy.


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

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

[YingChi Long via Phabricator via cfe-commits]

inclyc added a comment.

> I've left some comments in the review about examples of my concerns (it's not 
> an exhaustive review).

Thanks @aaron.ballman ! I didn't quite understand the original meaning of this 
code here (e.g. libclang), and I have now removed the relevant changes. I think 
this patch should replace the code that accidentally misuses of `assert(0)` 
with `llvm_unreachable()`.


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

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

[YingChi Long via Phabricator via cfe-commits]

inclyc updated this revision to Diff 466539.
inclyc added a comment.

Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551

Files:
  clang/include/clang/AST/Redeclarable.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/Interp/ByteCodeExprGen.cpp
  clang/lib/Analysis/CFG.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Basic/Targets/NVPTX.cpp
  clang/lib/CodeGen/CGHLSLRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/Multilib.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Frontend/Rewrite/RewriteModernObjC.cpp
  clang/lib/Frontend/Rewrite/RewriteObjC.cpp
  clang/lib/Frontend/SARIFDiagnostic.cpp
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Lex/PreprocessingRecord.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/SVals.cpp
  clang/tools/clang-refactor/TestSupport.cpp

Index: clang/tools/clang-refactor/TestSupport.cpp
===
--- clang/tools/clang-refactor/TestSupport.cpp
+++ clang/tools/clang-refactor/TestSupport.cpp
@@ -348,7 +348,7 @@
 if (!Matches[2].empty()) {
   // Don't forget to drop the '+'!
   if (Matches[2].drop_front().getAsInteger(10, ColumnOffset))
-assert(false && "regex should have produced a number");
+llvm_unreachable("regex should have produced a number");
 }
 Offset = addColumnOffset(Source, Offset, ColumnOffset);
 unsigned EndOffset;
@@ -365,7 +365,7 @@
   unsigned EndLineOffset = 0, EndColumn = 0;
   if (EndLocMatches[1].drop_front().getAsInteger(10, EndLineOffset) ||
   EndLocMatches[2].getAsInteger(10, EndColumn))
-assert(false && "regex should have produced a number");
+llvm_unreachable("regex should have produced a number");
   EndOffset = addEndLineOffsetAndEndColumn(Source, Offset, EndLineOffset,
EndColumn);
 } else {
Index: clang/lib/StaticAnalyzer/Core/SVals.cpp
===
--- clang/lib/StaticAnalyzer/Core/SVals.cpp
+++ clang/lib/StaticAnalyzer/Core/SVals.cpp
@@ -355,7 +355,7 @@
   break;
 }
 default:
-  assert(false && "Pretty-printed not implemented for this NonLoc.");
+  llvm_unreachable("Pretty-printed not implemented for this NonLoc.");
   break;
   }
 }
Index: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -192,7 +192,7 @@
   break;
 
 case ProgramPoint::BlockExitKind:
-  assert(false && "BlockExit location never occur in forward analysis.");
+  llvm_unreachable("BlockExit location never occur in forward analysis.");
   break;
 
 case ProgramPoint::CallEnterKind:
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -2438,7 +2438,7 @@
 MacroID ID = MacroInfosToEmit[I].ID;
 
 if (ID < FirstMacroID) {
-  assert(0 && "Loaded MacroInfo entered MacroInfosToEmit ?");
+  llvm_unreachable("Loaded MacroInfo entered MacroInfosToEmit ?");
   continue;
 }
 
@@ -5381,7 +5381,8 @@
 TypeIdx &Idx = TypeIdxs[T];
 if (Idx.getIndex() == 0) {
   if (DoneWritingDeclsAndTypes) {
-assert(0 && "New type seen after serializing all the types to emit!");
+llvm_unreachable(
+"New type seen after serializing all the types to emit!");
 return TypeIdx();
   }
 
@@ -5427,7 +5428,8 @@
   DeclID &ID = DeclIDs[D];
   if (ID == 0) {
 if (DoneWritingDeclsAndTypes) {
-  assert(0 && "New decl seen after serializing all the decls to emit!");
+  llvm_unreachable(
+  "New decl seen after serializing all the decls to emit!");
   return 0;
 }
 
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -7464,7 +7464,7 @@
   unsigned Index = ID - NUM_PREDEF_DECL_IDS;
 
   if (Index >= DeclsLoaded.size()) {
-assert(0 && "declaration ID out-of-range for AST file");
+llvm_unreachable("declaration ID out-of-range for AST file");
 Error("declaration ID out-of-range for AST file");
 return nullptr;
   }
@@ -7479,7 +7479,7 @@
   unsigned Index = ID - NUM_PREDEF_DECL_IDS;
 
   if (Index >= DeclsLoaded

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D135551#3846603 , @inclyc wrote:

> In D135551#3846592 , @aaron.ballman 
> wrote:
>
>> I don't know if that discussion reached a conclusion to move forward with 
>> this change -- my reading of the conversation was that efforts would be 
>> better spend on fuzzing instead of changing policy about using unreachable 
>> vs assert(0).
>>
>> In general, I'm a bit hesitant to make this change. On the one hand, it's 
>> logically no worse than using `assert(0)` in a release build (if you hit 
>> this code path, bad things are going to happen). But `__builtin_unreachable` 
>> can have time travel optimization effects that `assert` doesn't have, and so 
>> the kind of bad things which can happen are different between the two (and 
>> use of unreachable on reachable code paths might make for harder debugging 
>> in RelWithDebInfo builds). Historically, we've usually used 
>> `llvm_unreachable` for situations where we're saying "this code cannot be 
>> reached; if it can, something else has gone seriously wrong." For example, 
>> in code like: `int foo(SomeEnum E) { switch (E) { case One: return 1; 
>> default: return 2; } llvm_unreachable("getting here would be mysterious"); 
>> }` and we've used `assert(0)` for situations where we're saying "this code 
>> is possible to reach only when there were mistakes elsewhere which broke 
>> invariants we were relying on." The two situations are similar, but still 
>> different enough that I don't think we should wholesale change from one form 
>> to another.
>
>
>
>> but still different enough that I don't think we should wholesale change 
>> from one form to another.
>
> In general we can control the behavior here via `-DLLVM_UNREACHABLE_OPTIMIZE` 
> to choose making assumptions or traps (looks better than assertions to me).
>
> https://github.com/llvm/llvm-project/blob/50312ea133999cb2aad1ab9ef0ec39429a9427c5/llvm/include/llvm/Support/ErrorHandling.h#L125
>
> (This change was landed 7 months ago https://reviews.llvm.org/D121750)

That doesn't change the underlying concern that `assert(0)` and `unreachable` 
are used for different purposes and trying to unify those use cases might lose 
some expressivity in the code base.

I've left some comments in the review about examples of my concerns (it's not 
an exhaustive review).




Comment at: clang/tools/libclang/CIndex.cpp:5191
 
-  assert(false && "Invalid CXPrintingPolicyProperty");
+  llvm_unreachable("Invalid CXPrintingPolicyProperty");
   return 0;

This one is a bit questionable -- this is part of the C interface we expose, 
which is ABI stable, so the assert was alerting users to potential mismatches 
between versions of the library.



Comment at: clang/tools/libclang/CXCursor.cpp:1490
   CXGetTemplateArgumentStatus_Success) {
-assert(0 && "Unable to retrieve TemplateArgument");
+llvm_unreachable("Unable to retrieve TemplateArgument");
 return 0;

Each of these is actually reachable -- the asserts exist specifically to tell 
users of the C interface about problems with their assumptions. In each of 
these cases, the assert is avoiding the need for a local variable to assert on.



Comment at: clang/utils/TableGen/ClangSyntaxEmitter.cpp:120
 } else {
-  assert(false && "Unhandled Syntax kind");
+  llvm_unreachable("Unhandled Syntax kind");
 }

This should not be using unreachable -- the code is very much reachable. This 
should have changed from `assert` to `PrintFatalError`.


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

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

[YingChi Long via Phabricator via cfe-commits]

inclyc added a comment.

In D135551#3846592 , @aaron.ballman 
wrote:

> I don't know if that discussion reached a conclusion to move forward with 
> this change -- my reading of the conversation was that efforts would be 
> better spend on fuzzing instead of changing policy about using unreachable vs 
> assert(0).
>
> In general, I'm a bit hesitant to make this change. On the one hand, it's 
> logically no worse than using `assert(0)` in a release build (if you hit this 
> code path, bad things are going to happen). But `__builtin_unreachable` can 
> have time travel optimization effects that `assert` doesn't have, and so the 
> kind of bad things which can happen are different between the two (and use of 
> unreachable on reachable code paths might make for harder debugging in 
> RelWithDebInfo builds). Historically, we've usually used `llvm_unreachable` 
> for situations where we're saying "this code cannot be reached; if it can, 
> something else has gone seriously wrong." For example, in code like: `int 
> foo(SomeEnum E) { switch (E) { case One: return 1; default: return 2; } 
> llvm_unreachable("getting here would be mysterious"); }` and we've used 
> `assert(0)` for situations where we're saying "this code is possible to reach 
> only when there were mistakes elsewhere which broke invariants we were 
> relying on." The two situations are similar, but still different enough that 
> I don't think we should wholesale change from one form to another.



> but still different enough that I don't think we should wholesale change from 
> one form to another.

In general we can control the behavior here via `-DLLVM_UNREACHABLE_OPTIMIZE` 
to choose making assumptions or traps (looks better than assertions to me).

https://github.com/llvm/llvm-project/blob/50312ea133999cb2aad1ab9ef0ec39429a9427c5/llvm/include/llvm/Support/ErrorHandling.h#L125

(This change was landed 7 months ago https://reviews.llvm.org/D121750)


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I don't know if that discussion reached a conclusion to move forward with this 
change -- my reading of the conversation was that efforts would be better spend 
on fuzzing instead of changing policy about using unreachable vs assert(0).

In general, I'm a bit hesitant to make this change. On the one hand, it's 
logically no worse than using `assert(0)` in a release build (if you hit this 
code path, bad things are going to happen). But `__builtin_unreachable` can 
have time travel optimization effects that `assert` doesn't have, and so the 
kind of bad things which can happen are different between the two (and use of 
unreachable on reachable code paths might make for harder debugging in 
RelWithDebInfo builds). Historically, we've usually used `llvm_unreachable` for 
situations where we're saying "this code cannot be reached; if it can, 
something else has gone seriously wrong." For example, in code like: `int 
foo(SomeEnum E) { switch (E) { case One: return 1; default: return 2; } 
llvm_unreachable("getting here would be mysterious"); }` and we've used 
`assert(0)` for situations where we're saying "this code is possible to reach 
only when there were mistakes elsewhere which broke invariants we were relying 
on." The two situations are similar, but still different enough that I don't 
think we should wholesale change from one form to another.


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

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

[YingChi Long via Phabricator via cfe-commits]

inclyc updated this revision to Diff 466471.
inclyc added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fix CI issue


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551

Files:
  clang/include/clang/AST/Redeclarable.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/Interp/ByteCodeExprGen.cpp
  clang/lib/Analysis/CFG.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Basic/Targets/NVPTX.cpp
  clang/lib/CodeGen/CGHLSLRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/Multilib.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Frontend/Rewrite/RewriteModernObjC.cpp
  clang/lib/Frontend/Rewrite/RewriteObjC.cpp
  clang/lib/Frontend/SARIFDiagnostic.cpp
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Lex/PreprocessingRecord.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/SVals.cpp
  clang/tools/clang-refactor/TestSupport.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  clang/utils/TableGen/ClangSyntaxEmitter.cpp

Index: clang/utils/TableGen/ClangSyntaxEmitter.cpp
===
--- clang/utils/TableGen/ClangSyntaxEmitter.cpp
+++ clang/utils/TableGen/ClangSyntaxEmitter.cpp
@@ -117,7 +117,7 @@
 } else if (R.isSubClassOf("NodeType")) {
   NodeType = R.getName().str();
 } else {
-  assert(false && "Unhandled Syntax kind");
+  llvm_unreachable("Unhandled Syntax kind");
 }
   }
 
Index: clang/tools/libclang/CXCursor.cpp
===
--- clang/tools/libclang/CXCursor.cpp
+++ clang/tools/libclang/CXCursor.cpp
@@ -1487,12 +1487,12 @@
   TemplateArgument TA;
   if (clang_Cursor_getTemplateArgument(C, I, &TA) !=
   CXGetTemplateArgumentStatus_Success) {
-assert(0 && "Unable to retrieve TemplateArgument");
+llvm_unreachable("Unable to retrieve TemplateArgument");
 return 0;
   }
 
   if (TA.getKind() != TemplateArgument::Integral) {
-assert(0 && "Passed template argument is not Integral");
+llvm_unreachable("Passed template argument is not Integral");
 return 0;
   }
 
@@ -1504,12 +1504,12 @@
   TemplateArgument TA;
   if (clang_Cursor_getTemplateArgument(C, I, &TA) !=
   CXGetTemplateArgumentStatus_Success) {
-assert(0 && "Unable to retrieve TemplateArgument");
+llvm_unreachable("Unable to retrieve TemplateArgument");
 return 0;
   }
 
   if (TA.getKind() != TemplateArgument::Integral) {
-assert(0 && "Passed template argument is not Integral");
+llvm_unreachable("Passed template argument is not Integral");
 return 0;
   }
 
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -199,7 +199,7 @@
   if (clang_isDeclaration(Cursor.kind)) {
 const Decl *D = getCursorDecl(Cursor);
 if (!D) {
-  assert(0 && "Invalid declaration cursor");
+  llvm_unreachable("Invalid declaration cursor");
   return true; // abort.
 }
 
@@ -5188,7 +5188,7 @@
 return P->FullyQualifiedName;
   }
 
-  assert(false && "Invalid CXPrintingPolicyProperty");
+  llvm_unreachable("Invalid CXPrintingPolicyProperty");
   return 0;
 }
 
@@ -5280,7 +5280,7 @@
 return;
   }
 
-  assert(false && "Invalid CXPrintingPolicyProperty");
+  llvm_unreachable("Invalid CXPrintingPolicyProperty");
 }
 
 CXString clang_getCursorPrettyPrinted(CXCursor C, CXPrintingPolicy cxPolicy) {
Index: clang/tools/clang-refactor/TestSupport.cpp
===
--- clang/tools/clang-refactor/TestSupport.cpp
+++ clang/tools/clang-refactor/TestSupport.cpp
@@ -348,7 +348,7 @@
 if (!Matches[2].empty()) {
   // Don't forget to drop the '+'!
   if (Matches[2].drop_front().getAsInteger(10, ColumnOffset))
-assert(false && "regex should have produced a number");
+llvm_unreachable("regex should have produced a number");
 }
 Offset = addColumnOffset(Source, Offset, ColumnOffset);
 unsigned EndOffset;
@@ -365,7 +365,7 @@
   unsigned EndLineOffset = 0, EndColumn = 0;
   if (EndLocMatches[1].drop_front().getAsInteger(10, EndLineOffset) ||
   EndLocMatches[2].getAsInteger(10, EndColumn))
-assert(false && "regex should have produced a number");
+llvm_unreachable("regex should have produced a number");
   EndOffset = addEndLineOffsetAndEndColumn(Source, Offset, EndLineOffset,
End