[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8470-8473
+def err_expected_struct_pointer_argument : Error<
+  "expected pointer to struct as %ordinal0 argument to %1, found %2">;
+def err_expected_callable_argument : Error<
+  "expected a callable expression as %ordinal0 argument to %1, found %2">;
----------------
rsmith wrote:
> aaron.ballman wrote:
> > Should we combine these into:
> > ```
> > def err_builtin_dump_struct_expects : Error<
> >   "expected %select{pointer to struct|a callable expression}0 as %ordinal1 argument to %2, found %3">;
> > ```
> > to reduce the number of diagnostics we have to tablegen?
> I'm indifferent. That kind of refactoring will probably decrease the chance of these being reused for any other purpose, which might end up costing us more diagnostics, and will need a magic number in the source code to select between alternatives. Happy to do it if you think it's worthwhile, but I don't have any measurements for how much each diagnostic ID costs.
Eh, let's skip it for now. I bring it up on occasion because we do run out of diagnostic IDs on occasion, but that's more a systemic concern.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:572-574
+          // We don't know how to print this field. Print out its address
+          // with a format specifier that a smart tool will be able to
+          // recognize and treat specially.
----------------
rsmith wrote:
> aaron.ballman wrote:
> > Should we handle array types specifically, or should we continue to punt on those for now?
> I'd prefer to punt for now, if that's OK. I don't think this is hard to add, but it's probably clearer to do it in a separate patch. There are also some nontrivial questions there, eg: should we dump all elements or stop after some limit? Should we dump trailing zeroes in an integer array? How should we dump arrays of `char`? I'd prefer to not deal with those questions in this patch :)
Absolutely fine to punt on it, because I agree there's a bunch of questions to answer there. I was also thinking about what to do with flexible array members or other trailing array objects.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:573
+          // We don't know how to print this field. Print out its address
+          // with a format specifier that a smart tool will be able to
+          // recognize and treat specially.
----------------
rsmith wrote:
> yihanaa wrote:
> > Can we generate a warning diagnostic message when we see an unsupported type (perhaps temporarily not supported), what do you all think about?
> We could, but I'm leaning towards thinking that we shouldn't warn by default at least. That would get in the way of the C++ use case where the type actually is supported by the printing function; I'm not sure that we can easily tell those cases apart. And it's probably not all that useful to someone who just wants whatever information we can give them while debugging. I'd be OK with adding a warning that's off by default, though. What would be the intended use case here? Would an off-by-default warning satisfy it?
So long as we don't trigger UB, I think it's fine to not diagnose for unsupported types. But the UB cases do strike me as something we might want to warn about if we think they can be hit (specifically, these kinds of UB: https://eel.is/c++draft/cstdarg.syn#1).

As an example, do we properly handle dumping this case:
```
struct S {
  int &i;
  S(int &ref) : i(ref) {}
};
```
where the vararg argument is of reference type? I suppose another interesting question is:
```
struct S {
  int i;
};

void func() {
  S s;
  [=]() {
    __builtin_dump_struct(&s, ::printf);
  };
}
```
(is S.i "an entity resulting from a lambda capture"?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124221