[PATCH] D124221: Add new builtin __builtin_reflect_struct.

Mon Apr 25 13:13:01 PDT 2022

rsmith added a comment.

In D124221#3471645 <https://reviews.llvm.org/D124221#3471645>, @erichkeane wrote:

> In D124221#3470550 <https://reviews.llvm.org/D124221#3470550>, @aaron.ballman wrote:
>
>> In D124221#3469251 <https://reviews.llvm.org/D124221#3469251>, @rsmith wrote:
>>
>>> - 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`).
>>
>> +1, though I'm curious what we want to do about things like bit-fields (do we want to alert the caller that it's a bit-field and what the width is?), anonymous bit-fields, base classes vs fields of class type, etc.
>
> I suspect we'd want some sort of 'depth' parameter to that as well so we can handle the 'tabbing' correctly,

I was thinking that we'd pass hard-coded `"    "` strings to the `user_dump_function` for indentation (like we do now), rather than trying to make it customizable, so that, in particular, you can still pass in `printf` as the `user_dump_function` and have it "just work".

> and am also concerned with the things Aaron brought up (particularly the anonymous bit-fields/zero length bitfields), as they seem to be things that make current users persnickety.

Yeah. User concerns about the output format not being what they wanted really reinforce for me the fundamental problems with this builtin. But we can still make reasonable and principled choices here (eg, show the same information that would be in a corresponding compound literal).

>>> - 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
>>
>> +1, that seems pretty sensible for what is effectively a callback function.
>
> I am somewhat concerned with these as they get confusing/obtuse to use sometimes (particularly with variadic functions as the callback, where they cause strange non-compile-time errors with minor, seemingly innocuous changes), but am reasonably sure we could come up with some level of design that would make it not-awful.  If this was JUST C++, I'd say "we should not, and the user should use a functor/lambda for state", but, of course, this is C :/

I definitely understand that concern. I think it's important to allow at least one parameter -- I'd be OK with saying that you can pass either the signature of `printf` or that signature with a leading `void*`, but I think it's nicer to say we'll take whatever extra arguments are given and blindly pass them onto the given function, so that we can directly support `fprintf` and `dprintf`. (You'd still need a wrapper in order to be able to call `snprintf`, though, in order to move the pointer forward and correspondingly decrease the remaining buffer length.) But as long as we provide *some* way to pass arbitrary information to the callback, I think that satisfies the use case.

> The BOFH-compiler-guy part of me (B-Compiler-Dev-FH ?) thinks we should start randomizing the output to discourage this behavior, but I suspect that would be an arms-race we would ultimately lose.

:-) I think the best thing we can do to discourage people from using this dumping facility for reflection is to provide builtins to implement the C++ reflection functionality as soon as we can after we think it's ready -- and ideally to make sure that those builtins support at least basic use cases in C. (The latter might require that we wait until C's constant evaluation functionality improves, but at least some in WG14 are pushing hard for that.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124221