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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
