aaronpuchert added inline comments.
================
Comment at: clang/lib/AST/ASTContext.cpp:12367
+ case Type::Class:
\
+ llvm_unreachable("Unexpected " Kind ": " #Class);
+
----------------
mizvekov wrote:
> aaronpuchert wrote:
> > mizvekov wrote:
> > > davrec wrote:
> > > > mizvekov wrote:
> > > > > davrec wrote:
> > > > > > mizvekov wrote:
> > > > > > > davrec wrote:
> > > > > > > > Could we just return `X` here? Would that just default to the
> > > > > > > > old behavior instead of crashing whenever unforeseen cases
> > > > > > > > arise?
> > > > > > > No, I think we should enforce the invariants and make sure we are
> > > > > > > handling everything that can be handled.
> > > > > > >
> > > > > > > Classing `TemplateTypeParm` as sugar-free was what was wrong and
> > > > > > > we missed this on the review.
> > > > > > There might always going to be a few rare corner cases vulnerable
> > > > > > to this though, particularly as more types are added and the people
> > > > > > adding them don't pay strict attention to how to incorporate them
> > > > > > here, and don't write the requisite tests (which seem very
> > > > > > difficult to foresee and produce). When those cases arise we will
> > > > > > be crashing even though we could produce a perfectly good program
> > > > > > with the intended semantics; the only thing that would suffer for
> > > > > > most users is slightly less clear diagnostic messages for those
> > > > > > rare cases. I think it would be better to let those cases
> > > > > > gradually percolate to our attention via bug reports concerning
> > > > > > those diagnostics, rather than inconveniencing the user and
> > > > > > demanding immediate attention via crashes. Or am I missing
> > > > > > something?
> > > > > I could on the same argument remove all asserts here and just let the
> > > > > program not crash on unforeseen circumstances.
> > > > >
> > > > > On the other hand, having these asserts here helps us catch bugs not
> > > > > only here, but in other parts of the code. For example uniquing /
> > > > > canonicalization bugs.
> > > > >
> > > > > If someone changes the properties of a type so that the assumptions
> > > > > here are not valid anymore, it's helpful to have that pointed out
> > > > > soon.
> > > > >
> > > > > Going for as an example this specific bug, if there werent those
> > > > > asserts / unreachables in place and we had weakened the validation
> > > > > here, it would take a very long time for us to figure out we were
> > > > > making the wrong assumption with regards to TemplateTypeParmType.
> > > > >
> > > > > I'd rather figure that out sooner on CI / internal testing rather
> > > > > than have a bug created by a user two years from now.
> > > > Yes its nicer to developers to get stack traces and reproductions
> > > > whenever something goes wrong, and crashing is a good way to get those,
> > > > but users probably won't be so thrilled about it. Especially given
> > > > that the main selling point of this patch is that it makes diagnostics
> > > > nicer for users: isn't it a bit absurd to crash whenever we can't
> > > > guarantee our diagnostics will be perfect?
> > > >
> > > > And again the real problem is future types not being properly
> > > > incorporated here and properly tested: i.e. the worry is that this will
> > > > be a continuing source of crashes, even if we handle all the present
> > > > types properly.
> > > >
> > > > How about we leave it as an unreachable for now, to help ensure all the
> > > > present types are handled, but if months or years from now there
> > > > continue to be crashes due to this, then just return X (while maybe
> > > > write something to llvm::errs() to encourage users to report it), so we
> > > > don't make the perfect the enemy of the good.
> > > It's not about crashing when it won't be perfect. We already do these
> > > kinds of things, see the FIXMEs around the TemplateArgument and
> > > NestedNameSpecifier, where we just return something worse than we wish
> > > to, just because we don't have time to implement it now.
> > >
> > > These unreachables and asserts here are about testing / documenting our
> > > knowledge of the system and making it easier to find problems. These
> > > should be impossible to happen in correct code, and if they do happen
> > > because of mistakes, fixing them sooner is just going to save us
> > > resources.
> > >
> > > `llvm::errs` suggestion I perceive as out of line with current practice
> > > in LLVM, we don't and have a system for producing diagnostics for results
> > > possibly affected by FIXME and TODO situations and the like, as far as I
> > > know, and I am not aware of any discussions in that direction. I expect a
> > > lot of complexity and noise if we went this way.
> > > And I think this would have even less chance of working out if we started
> > > to incorporate the reporting of violation of invariants into that.
> > >
> > > I think even just using raw `llvm::errs` on clang would be incorrect per
> > > design, and could likely break users that parse our output.
> > >
> > > I think if months and years from now, if someone stumbled upon an assert
> > > firing here and, instead of understanding what is happening and then
> > > fixing the code, just removed / weakened the assert, that would simply
> > > not be good and I hope a reviewer would stop that from happening :)
> > I tend to agree that an assertion is appropriate for this. But you could
> > turn this into
> > ```
> > assert(false && "...");
> > return X;
> > ```
> > which would still assert, but fall back to something "reasonable" in builds
> > without assertions. The issue with `llvm_unreachable` is that it translates
> > to `__builtin_unreachable()` in builds without assertions, and the
> > optimizer takes advantage of that quite heavily. That's why
> > `llvm_unreachable` is better left to places where we're pretty sure they're
> > unreachable unless something went horribly wrong, such as after switches
> > that handle all enumeration values.
> I see, that is reasonable.
>
> But grepping for `assert(false` in the code base, I see that they are pretty
> rare, accounting for a bit less than 0.5% in `clang/` and 0.3% in `llvm/`,
> compared to the total number of asserts. They occur fifty times less than
> plain llvm_unreachables. I don't remember ever seeing them in practice, so I
> was not aware this was something we would do.
>
> I feel like in these cases where we are using llvm_unreachable in this patch,
> I am 100% sure they are really unreachable, unless something went horribly
> wrong. But I mean I could be completely wrong as well, I must admit, so maybe
> in those cases that is what went horribly wrong? (Me being wrong I mean)
It depends on how likely you think it is that someone inadvertently violates
the invariant without noticing (even after running the tests). For internal
invariants or things unlikely to miss in a test, `llvm_unreachable` should be
fine. The less certain you are that mistakes would be hard to make or easy to
find, the more preferable would be a fallback. If you think this was a one-off
blunder and is unlikely to reoccur, you can also stick to `llvm_unreachable`.
So this was just a suggestion, and since I merely skimmed over the code I'm not
the best to judge.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111283/new/
https://reviews.llvm.org/D111283
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
