Thanks for your review @erichkeane!

This is unfortunately true, unless we generate some code that calculates the length of the string at runtime. I find it difficult to draw the line between defining this built-in functionality and developers developing printf-like functionality, if built-in does too much, flexibility will decrease, if built-in does less, (features like length limits) then develop personnel may need to do some work. o(≧口≦)o

I found LLDB have this feature. LLDB uses "..." to indicate that only part of the string is now printed, and it also have a command to modify length limit. This approach is similar to the idea of @erichkeane, can we do the same？what do you all think?

typedef signed char int8_t;
typedef unsigned char uint8_t;

Do we need to do argument promotion here，ahh...maybe i understood wrong

This implementation looks good to me😁

Thank you all, I have commit in eaca933c59fd61b3df4697b5fae0eeec67acfaeb

Thanks for review @rsmith, @erichkeane and @aaron.ballman , I have add a newline back to the end of the test, and waiting for ci....

Add the newline back to the end of the test.

While we'd usually be happy to take the fix-in-hand and apply it, part of the discussion on the other thread is whether to remove __builtin_dump_struct entirely. Because of that, I don't think we should make substantial changes in this area until it's clear we're keeping the builtin.

In D122920#3472379, @asoffer wrote:

In D122920#3471865, @yihanaa wrote:

In D122920#3471654, @erichkeane wrote:

@yihanaa : I'd suggest seeing the conversation that @rsmith @aaron.ballman and I are having about this builtin here: https://reviews.llvm.org/D124221

In general it looks like the three of us think that this builtin needs an overhaul in implementation/functionality in order to be further useful. While we very much appreciate your attempts to try to improve it, we believe there needs to be larger-scale changes.

Thanks for your reply, I would like to get involved and help if needed

While I eagerly await __builtin_reflect_struct, this change still provides significant value in fixing a regression with __builtin_dump_struct. Should we proceed with this change and that proposal independently?

Agree

Thank you for inviting me to a discussion on this topic. I have something very confusing as follows:

In D122920#3471654, @erichkeane wrote:

@yihanaa : I'd suggest seeing the conversation that @rsmith @aaron.ballman and I are having about this builtin here: https://reviews.llvm.org/D124221

In general it looks like the three of us think that this builtin needs an overhaul in implementation/functionality in order to be further useful. While we very much appreciate your attempts to try to improve it, we believe there needs to be larger-scale changes.

Should we use PrintingPolicy.Indentation instead of 4 hardcoded in dumpRecord?

Optimize the code according to @rsmith 's suggestion, and use analyze_printf::PrintfSpecifier::fixType instead of this static DenseMap to find the printf format specifier.

Could you please help me review the code？@rsmith，thanks😊

Improve comment

I'll just add that in the recursive call, for record type, it will just print what's between {...}
Eg:

int printf(const char *, ...);

In D122920#3426556, @erichkeane wrote:

I don't particularly get what the 'fix' is here to Richard's issue. Additionally, I think this is showing me the absolute desperate need we have for some sort of 'text-checking' tests for this feature, checking vs the IR is too unreadable.

Can you guys please help me review this patch? 😁

Reformat code

Reformat code.

In D122822#3422463, @erichkeane wrote:

In D122822#3421089, @yihanaa wrote:

I also agree that this is a useful debugging builtin, and no program logic should be forced to depend on the output of this builtin. It's just used to print out some useful debugging information

FWIW, @rsmith is the clang-codeowner, so he's the one to convince (and @rjmccall is also a very senior reviewer). So I wouldn't be able to approve this patch without their consent/agreement.

I also agree that this is a useful debugging builtin, and no program logic should be forced to depend on the output of this builtin. It's just used to print out some useful debugging information

In D122822#3420162, @rsmith wrote:

I'm concerned about the direction of this patch given @rjmccall's comments on https://reviews.llvm.org/D112626 -- presumably the way we'd want to address those comments would be to convert a __builtin_dump_struct(a, f) call into a single f("...", a.x, a.y, a.z) call in Sema, and that approach doesn't seem compatible with generating code to loop over array elements.

I'm also concerned that this builtin is making a lot of design decisions on behalf of the programmer, meaning that either it does exactly what you want or it's not suitable for your use and there's not much you can do about it. A different design that let the programmer choose whether to recurse into structs and arrays and similarly let the programmer choose how to format the fields would seem preferable, but I'm not confident there's a good way to expose that in C (though in C++ there seem to be good designs to choose from). As an example of how that manifests here, the choice to print each element as a separate line for a struct member of type const char str[256]; is probably going to make the output very unreadable, but choosing to print every array of char using "%s" will be equally bad for arrays that don't contain text -- the "right" answer for a dumping facility probably involves checking whether the array contains only printable characters and trimming a trailing sequence of zeroes, but that seems like vastly more complexity than we'd want in code generated by a builtin.

There is another question, which way should I print for an empty array?

Thanks for your suggestion @erichkeane !

Thank you for the reviews @erichkeane. I've applied for commit access and commit in 907d3acefc3bdd6eb83f21589c6473ca7e88b3eb.

In D122704#3416538, @erichkeane wrote:

This seems fine to me. I won't likely have time to commit this for a while, but you should be able to apply for commit-rights now and do it yourself.

In D122670#3414574, @erichkeane wrote:

Still LGTM.

Add this change into ReleaseNotes.

In D122670#3414549, @erichkeane wrote:

I think Aaron wants release notes for just about everything :) Mind tossing one on this patch? I'll commit for you after that.

In D122668#3414445, @erichkeane wrote:

2 small bits, otherwise LGTM.

Fixed "Description" issue and removed useless newlines

In D122662#3414364, @erichkeane wrote:

In D122662#3414319, @yihanaa wrote:

In D122662#3414284, @erichkeane wrote:

I'd also suggest splitting into the '3' things that you're trying to accomplish above. The CGBuiltin.cpp code has way too much going on to reasonably review.

In D122662#3414284, @erichkeane wrote:

I'd also suggest splitting into the '3' things that you're trying to accomplish above. The CGBuiltin.cpp code has way too much going on to reasonably review.

Maybe I should split the third thing into another separate patch, the first thing needs to modify the indent when adding constant array support, that is the third thing (modify the indent), what do you think?

I would suggest piling them from #3, to #2 to #1. That is: "Change the anonymous printing" in the first, add the 'sub-element-tabbing' in 2nd, and 'add constant array support' in 3rd.

In D122662#3414284, @erichkeane wrote:

I'd also suggest splitting into the '3' things that you're trying to accomplish above. The CGBuiltin.cpp code has way too much going on to reasonably review.

In D122662#3414284, @erichkeane wrote:

I'd also suggest splitting into the '3' things that you're trying to accomplish above. The CGBuiltin.cpp code has way too much going on to reasonably review.

In D122662#3414199, @erichkeane wrote:

This looks to be quite the large patch, can you split this up into individual patches? It isn't clear to me what all is going on everywhere.

split another patch to fix ReleaseNotes.

Thanks for review @erichkeane ,maybe i should split this patch.

Remove unnecessary code format

In D122248#3406180, @erichkeane wrote:

In D122248#3406162, @yihanaa wrote:

In D122248#3406145, @aaron.ballman wrote:

LGTM aside from a tiny nit with one of the release notes.

I'd be happy to fix it

In D122248#3406145, @aaron.ballman wrote:

LGTM aside from a tiny nit with one of the release notes.

Don't worry about it, I'll make the change as part of committing.

Put the dump format changes under the "Non-comprehensive list of changes in this release" heading instead.

In D122248#3406145, @aaron.ballman wrote:

LGTM aside from a tiny nit with one of the release notes.

In D122248#3406117, @erichkeane wrote:

LGTM! Please give Aaron a few hours (perhaps until tomorrow?) to take 1 last look before committing.

Also, if you lack commit rights and need someone to commit for you, please provide the name + email address you'd like it committed under.

Waitting for CI...

Use `FD->getDeclName().empty() instead of FD->getNameAsString().empty()`
Add a the format changes to release notes

Maybe add the changes under "Attribute Changes in Clang"?

In D122248#3405866, @aaron.ballman wrote:

In D122248#3405644, @yihanaa wrote:

In D122248#3405166, @erichkeane wrote:

In D122248#3405062, @aaron.ballman wrote:

In D122248#3403734, @yihanaa wrote:

What if we don't emit '=' for zero-width bitfield, like this:

struct Bar {
unsigned c : 1;
unsigned : 3;
unsigned : 0;
unsigned b;
};

struct Bar {
unsigned int c : 1 = 0
unsigned int   : 3 = 0
unsigned int   : 0
unsigned int b = 0
}

What do you all think?

I like this idea best of all!

Agreed!

@erichkeane @aaron.ballman
Previously we used FieldDecl->getNameAsString to get the field name, but the comments for this function indicate that it is Deprecated，and suggestion move clients to getName().

The FieldDecl->getName() return the anonymous inner struct's name like:

struct T3A {
  union {
    int a;
    char b[4];
  };
};

struct T3A {
union T3A::(anonymous at ./builtin_dump_struct.c:77:5) {
    int a = 42
    char[4] b = 0x2a
    }
}

what do you all think?

I would probably keep using getNameAsString() as I don't think the extra information helps overly much. That interface was marked deprecated 13 years ago, so don't feel bad for making use of it.

Support dump bitwidth of bitfields, and unnamed bitfields.
for example:

struct Bar {
unsigned c : 1;
unsigned : 3;
unsigned : 0;
unsigned b;
};

In D122248#3405166, @erichkeane wrote:

In D122248#3405062, @aaron.ballman wrote:

In D122248#3403734, @yihanaa wrote:

What if we don't emit '=' for zero-width bitfield, like this:

struct Bar {
unsigned c : 1;
unsigned : 3;
unsigned : 0;
unsigned b;
};

struct Bar {
unsigned int c : 1 = 0
unsigned int   : 3 = 0
unsigned int   : 0
unsigned int b = 0
}

What do you all think?

I like this idea best of all!

Agreed!

In D122248#3403698, @erichkeane wrote:

I'm sorry I misunderstood what you meant @aaron.ballman.

Can we follow the lead of LLVM IR？it use 'undef'
for example:

struct T6A {
    unsigned a : 1;
    unsigned  : 0;
    unsigned c : 1;
};

@__const.foo.a = private unnamed_addr constant %struct.T6A { i8 1, [3 x i8] undef, i8 1, [3 x i8] undef }, align 4

I misunderstood him too, he told me off line :)

I guess I would be 'ok' with undef, though that has a different meaning (it means, this has an arbitrary value). In this case, it has NO value, which is somewhat different.

In D122248#3403637, @erichkeane wrote:

In D122248#3403636, @yihanaa wrote:

In D122248#3403518, @aaron.ballman wrote:

In D122248#3403478, @erichkeane wrote:

If it is ok, I think we should probably change the format of the 'dump' for fields. Using the colon to split up the field from the value is unfortunate, may I suggest replacing it with '=' instead? As well as printing the size after a colon. So for:

void foo(void) {
  struct Bar {
    unsigned c : 1;
    unsigned : 3;
    unsigned : 0;
    unsigned b;
  };

  struct Bar a = {
    .c = 1,
    .b = 2022,
  };

  __builtin_dump_struct(&a, &printf);
}

Output:

struct Bar {
unsigned int c : 1 = 1
unsigned int : 3  = 0
unsigned int : 0 = 
unsigned int b = 2022
}

What do you all think?

I think that's a good idea for clarity. For the case where we have no value, I wonder if we want to do something like: unsigned int : 0 = <uninitialized> (or something else to make it exceptionally clear that there's nothing missing after the =)?

how

In D122248#3403518, @aaron.ballman wrote:

In D122248#3403478, @erichkeane wrote:

If it is ok, I think we should probably change the format of the 'dump' for fields. Using the colon to split up the field from the value is unfortunate, may I suggest replacing it with '=' instead? As well as printing the size after a colon. So for:

void foo(void) {
  struct Bar {
    unsigned c : 1;
    unsigned : 3;
    unsigned : 0;
    unsigned b;
  };

  struct Bar a = {
    .c = 1,
    .b = 2022,
  };

  __builtin_dump_struct(&a, &printf);
}

Output:

struct Bar {
unsigned int c : 1 = 1
unsigned int : 3  = 0
unsigned int : 0 = 
unsigned int b = 2022
}

What do you all think?

I think that's a good idea for clarity. For the case where we have no value, I wonder if we want to do something like: unsigned int : 0 = <uninitialized> (or something else to make it exceptionally clear that there's nothing missing after the =)?

How to judge whether this field is initialized? Maybe this memory has been initialized by memset

He means a special-case for the 0-size bitfield, which HAS no value (actually, wonder if this is a problem with the no-unique-address types as well?). I might suggest N/A instead of uninitialized, but am open to bikeshedding.

In D122248#3403518, @aaron.ballman wrote:

In D122248#3403478, @erichkeane wrote:

If it is ok, I think we should probably change the format of the 'dump' for fields. Using the colon to split up the field from the value is unfortunate, may I suggest replacing it with '=' instead? As well as printing the size after a colon. So for:

void foo(void) {
  struct Bar {
    unsigned c : 1;
    unsigned : 3;
    unsigned : 0;
    unsigned b;
  };

  struct Bar a = {
    .c = 1,
    .b = 2022,
  };

  __builtin_dump_struct(&a, &printf);
}

Output:

struct Bar {
unsigned int c : 1 = 1
unsigned int : 3  = 0
unsigned int : 0 = 
unsigned int b = 2022
}

What do you all think?

I think that's a good idea for clarity. For the case where we have no value, I wonder if we want to do something like: unsigned int : 0 = <uninitialized> (or something else to make it exceptionally clear that there's nothing missing after the =)?

In D122248#3403478, @erichkeane wrote:

If it is ok, I think we should probably change the format of the 'dump' for fields. Using the colon to split up the field from the value is unfortunate, may I suggest replacing it with '=' instead? As well as printing the size after a colon. So for:

void foo(void) {
  struct Bar {
    unsigned c : 1;
    unsigned : 3;
    unsigned : 0;
    unsigned b;
  };

  struct Bar a = {
    .c = 1,
    .b = 2022,
  };

  __builtin_dump_struct(&a, &printf);
}

Output:

struct Bar {
unsigned int c : 1 = 1
unsigned int : 3  = 0
unsigned int : 0 = 
unsigned int b = 2022
}

What do you all think?

In D122248#3403349, @erichkeane wrote:

In D122248#3403343, @yihanaa wrote:

In D122248#3403315, @aaron.ballman wrote:

In D122248#3403143, @yihanaa wrote:

1. Support zero-width bitfield, named bitfield and unnamed bitfield.
2. Add a release notes.

The builtin function __builtin_dump_struct behaves for zero-width bitfield and unnamed bitfield as follows

int printf(const char *fmt, ...);

void foo(void) {
  struct Bar {
    unsigned c : 1;
    unsigned : 3;
    unsigned : 0;
    unsigned b;
  };

  struct Bar a = {
    .c = 1,
    .b = 2022,
  };

  __builtin_dump_struct(&a, &printf);
}

int main() {
  foo();
  return 0;
}

Output:

struct Bar {
unsigned int c : 1
unsigned int  : 0
unsigned int b : 2022
}

Thank you for the release note and additional test coverage. I'm wondering why we handle the zero-width bit-field differently from the anonymous one (e.g., why do we not have unsigned int : 3 before the unsigned int : 0? It seems a bit odd to drop that from the output.

Thanks, I don't know what the value of this zero-width bitfield should output, can it be a empty value as same as unnamed-bitfield’ she field name?

for example:

struct Bar {
unsigned int c : 1
unsigned int  : 0
unsigned int  :
unsigned int b : 2022
}

I would definitely expect this, yes.

In D122248#3403315, @aaron.ballman wrote:

In D122248#3403143, @yihanaa wrote:

1. Support zero-width bitfield, named bitfield and unnamed bitfield.
2. Add a release notes.

The builtin function __builtin_dump_struct behaves for zero-width bitfield and unnamed bitfield as follows

int printf(const char *fmt, ...);

void foo(void) {
  struct Bar {
    unsigned c : 1;
    unsigned : 3;
    unsigned : 0;
    unsigned b;
  };

  struct Bar a = {
    .c = 1,
    .b = 2022,
  };

  __builtin_dump_struct(&a, &printf);
}

int main() {
  foo();
  return 0;
}

Output:

struct Bar {
unsigned int c : 1
unsigned int  : 0
unsigned int b : 2022
}

Thank you for the release note and additional test coverage. I'm wondering why we handle the zero-width bit-field differently from the anonymous one (e.g., why do we not have unsigned int : 3 before the unsigned int : 0? It seems a bit odd to drop that from the output.