[PATCH] D124221: Add new builtin __builtin_reflect_struct.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D124221#3468706 <https://reviews.llvm.org/D124221#3468706>, @aaron.ballman wrote:

> Thank you for looking into this! I've also thought that the `__builtin_dump_struct` implementation hasn't been quite as robust as it could be. However, the primary use case for that builtin has been for kernel folks to debug when they have no access to an actual debugger -- it being a super limited interface is actually a feature rather than a bug, in some ways.

I'm more concerned about the design of `__builtin_dump_struct` than the implementation -- we can always fix the implementation. Right now the design is hostile to any use other than the very simplest one where the function you give it is essentially `printf`. You can't even dump to a log file or to a string without having to fight the design of the builtin. And if you want anything other than the exact recursion and formatting choices that we've arbitrarily made, you're out of luck. And of course, if you're in C++, you'll want to be able to print out `std::string` and `std::optional<int>` and similar types too, which are not supported by the current design at all. Perhaps this is precisely what a specific set of kernel folks want for a specific use case, but if the only purpose is to address that one use case, then I don't really see how we can justify keeping this builtin in-tree. So I think we should either expand the scope beyond the specific kernel folks or we should remove it.

> I think what you're designing here is straight-up reflection, which is a different use case. If we're going to get something that's basically only usable in C++, I'm not certain there's much value in adding builtins for reflection until WG21 decides what reflection looks like, and then we can design around that. (Personally, I think designing usable reflection interfaces for C would be considerably harder but would provide considerably more benefit to users given the lack of reflection capabilities. There's almost no chance WG14 and WG21 would come up with the same interfaces for reflection, so I think we've got some opportunity for exploration here.)

What I set out to build is something that lets you implement struct dumping without all the shortcomings I see in `__builtin_dump_struct`. I think it's inevitable that that ends up being substantially a struct reflection system, and indeed `__builtin_dump_struct` is a reflection system, too, just a really awkward one -- people have already started parsing its format strings to extract struct field names, for example. (The usage I've seen actually *is* struct dumping, but that usage can't use `__builtin_dump_struct` directly because of its limitations, so `__builtin_dump_struct` is just used to extract the field names, and the field values and types are extracted via structured bindings instead.)

I do agree that we shouldn't be providing a full reflection mechanism here, given both that one is coming anyway, and that whatever we design, people will inevitably ask for more, and we don't want to be maintaining our own reflection technology.

So, I think we should either roll back `__builtin_dump_struct` or fix forward. This patch attempted to do the latter, but maybe it's gone too far in the direction of reflection. I think we can address most of my concerns with `__builtin_dump_struct` without a new builtin, by incorporating things from this implementation as follows:

- Desugar it in Sema rather than in CodeGen -- this is necessary to enable following things as well as for constant evaluation
- Take any callable as the printing function and pass it the fields with their correct types, so that a C++ implementation can dispatch based on the type and print the values of types that we don't hard-code (eg, we'd generate calls like `user_dump_function("%s = %p", "field_name", &p->field_name);`, and a smart C++ formatting function can preserve the field type and do something suitable when formatting a corresponding `%p`).
- Take additional arguments to be passed onto the provided function, so that C users can easily format to a different file or to a  string or whatever else

The main concern that I think we can't address this way is that `__builtin_dump_struct` decides for itself how to format the struct. But if we keep that property, that might actually help to keep us in the bounds of a builtin that's good for dumping but nothing else?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124221