[Lldb-commits] [PATCH] D113673: [lldb] Unwrap the type when dereferencing the value

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

Thanks a lot for fixing this, could you point to D103532 <https://reviews.llvm.org/D103532> as the cause for the regression in the commit message?

This LGTM in general, but I think there could still be some obscure cases where this could fail.

So the switch statement this code is in (and which detects whether its a reference type) is defined as:

  clang::QualType parent_qual_type(
      RemoveWrappingTypes(GetCanonicalQualType(type)));
  switch (parent_qual_type->getTypeClass()) {

`GetCanonicalQualType` should strip all the outer and inner type sugar there is. And `RemoveWrappingTypes` removes *most* of the outer type sugar (which isn't necessary anymore as it should be all gone), but also makes types non-atomic (for simplicity reasons).

Meanwhile in our reference-case code we try to remove the outer type sugar (and atomic) via `RemoveWrappingTypes`. And if this doesn't end up in a ReferenceType the cast fails.

So IIUC there is still a possibility that we fail to cast here in case `GetCanonicalQualType` removes more outer sugar than `RemoveWrappingTypes`, which I think can only happen for type sugar classes not implemented by `RemoveWrappingTypes` (e.g., a quick grep tells me AttributedType and some template stuff is sugar that we don't handle).

So I would propose we do the following:

1. This patch can land now as-is to fix the regression.
2. It would be nice if we could expand the test with a few more variations of sugar. E.g., typedef to a reference of a typedef type and all that stuff.
3. I think we can simplify `RemoveWrappingTypes` to remove all type sugar (not just our small list we maintain there). That should eliminate the obscure cases and we no longer need to maintain a list of type sugar there.



================
Comment at: lldb/test/API/lang/cpp/dereferencing_references/main.cpp:10
+  // typedef of a reference
+  td_int_ref td = i;
+
----------------
nit: Could you give this a more unique name such as `td_to_ref_type`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113673