This is an archive of the discontinued LLVM Phabricator instance.

[Clang][CodeGen]Fix __builtin_dump_struct missing record type field name
ClosedPublic

Authored by yihanaa on Apr 1 2022, 10:42 AM.

Details

Summary

Thanks for @rsmith to point this. I'm sorry for introducing this bug.
See @rsmith 's comment in https://reviews.llvm.org/D122248
Eg:(By @rsmith ) https://godbolt.org/z/o7vcbWaEf
I have added a test case
struct:

struct U19A {
    int a;
  };
  struct U19B {
    struct U19A a;
  };

  struct U19B a = {
    .a.a = 2022
  };

Dump result:

struct U19B {
    struct U19A a = {
        int a = 2022
    }
}

Diff Detail

Event Timeline

yihanaa created this revision.Apr 1 2022, 10:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2022, 10:42 AM
yihanaa requested review of this revision.Apr 1 2022, 10:42 AM
yihanaa updated this revision to Diff 419923.Apr 1 2022, 8:24 PM

Reformat code.

yihanaa updated this revision to Diff 419928.Apr 1 2022, 8:43 PM

Reformat code

Can you guys please help me review this patch? 😁

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.

clang/lib/CodeGen/CGBuiltin.cpp
2046–2047

Not a fan of bool parameters, they make calls harder to read.

2116

This comment isn't clear to me, can you clarify? I thought the issue that @rsmith reported was that we dumped too types rarely, but it looks like you're taking cases where we already dumped the typename and stopped doing so?

2125
2697
yihanaa added a comment.EditedApr 4 2022, 9:41 AM

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.

Thanks for review! The issue that @rsmith reported was when the struct field is a record type, the __builtin_dumpr_struct no longer prints the field name.
Eg:
The code:

#include <stdio.h>

struct A {
    int x;
};

struct B {
    struct A a;
};


int main() {
    struct B x = {0};
    __builtin_dump_struct(&x, &printf);
}

The clang trunk:(This missing field name 'a' for type struct A), our expected output is 'struct A a = {')

struct B {
    struct A {
        int x = 0
    }
}

After this patch:

struct B {
    struct A a = {
        int x = 0
    }
}

I try to improve this comment.

yihanaa added inline comments.Apr 4 2022, 9:47 AM
clang/lib/CodeGen/CGBuiltin.cpp
2046–2047

I can add comments for this parameter or this function, what do you think, or do you have a better idea? I would like to improve it.

2125

Agree!I will modify it later

2697

Agree!

erichkeane added inline comments.Apr 4 2022, 9:50 AM
clang/lib/CodeGen/CGBuiltin.cpp
2046–2047

I prefer using an 'enum' for these sorts of things, we do that elsewhere. It seems that what you ACTUALLY care about here is whether this is a 'top level', and thus has a FieldName to print, right? So you actually want to dumpFieldName? Or am I still misunderstanding something?

yihanaa added inline comments.Apr 4 2022, 10:00 AM
clang/lib/CodeGen/CGBuiltin.cpp
2046–2047

I prefer using an 'enum' for these sorts of things, we do that elsewhere. It seems that what you ACTUALLY care about here is whether this is a 'top level', and thus has a FieldName to print, right? So you actually want to dumpFieldName? Or am I still misunderstanding something?

Yeah, you are right. A little different is that this patch will DO NOT dump the typename, field name and '=' in recursive dumpRecord call.

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

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

struct A {
    int x;
};

struct B {
    struct A a;
    struct C {
      int b;
      union X {
        int i;
        int j;
      } z;
    } bar;
};


int main() {
    struct B x = {0};
    __builtin_dump_struct(&x, &printf);
}

output:

struct B {
    struct A a = {
        int x = 0
    }
    struct C bar = {
        int b = 0
        union X z = {
            int i = 0
            int j = 0
        }
    }
}
yihanaa added inline comments.Apr 4 2022, 10:46 AM
clang/lib/CodeGen/CGBuiltin.cpp
2046–2047

Yeah, you are right.

yihanaa updated this revision to Diff 420254.Apr 4 2022, 11:18 AM

Improve comment

Could you please help me review the code?@rsmith,thanks😊

rsmith added inline comments.Apr 21 2022, 2:20 PM
clang/lib/CodeGen/CGBuiltin.cpp
2046–2047

Instead of passing a flag that suppresses the first section of this function, it would be clearer to split this up into two parts:

  1. A function that dumps a type name (struct A or int or whatever).
  2. A function that dumps a struct value ({x = 1, y = 2}).

Then when printing the overall value, you can call (1) then (2), and when printing each field, you can call (1), then print the field name, then call (2) for a struct value (and do whatever else makes sense for other kinds of values).

yihanaa added inline comments.Apr 23 2022, 4:35 AM
clang/lib/CodeGen/CGBuiltin.cpp
2046–2047

Instead of passing a flag that suppresses the first section of this function, it would be clearer to split this up into two parts:

  1. A function that dumps a type name (struct A or int or whatever).
  2. A function that dumps a struct value ({x = 1, y = 2}).

Then when printing the overall value, you can call (1) then (2), and when printing each field, you can call (1), then print the field name, then call (2) for a struct value (and do whatever else makes sense for other kinds of values).

Thanks for your review! I agree with your suggestion, and i try to improve code.

2074–2075

I have an idea, can we use analyze_printf::PrintfSpecifier::fixType instead of this static DenseMap to find the printf format specifier? what do you all think?

yihanaa updated this revision to Diff 424718.EditedApr 23 2022, 7:14 AM

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.

yihanaa marked 6 inline comments as done.Apr 23 2022, 7:16 AM

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

@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.

@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

@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?

@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

This comment was removed by yihanaa.

@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

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.

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.

Thanks for take a look, I agree to wait for the outcome of the discussion

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.

Thanks for take a look, I agree to wait for the outcome of the discussion

I think I disagree with Aaron. While, YES, we are likely to delete this in the near future, it doesn't mean we should leave it in a broken state in the meantime. WHILE all of this is likely to be deleted, it has value until we delete it.

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.

Thanks for take a look, I agree to wait for the outcome of the discussion

I think I disagree with Aaron. While, YES, we are likely to delete this in the near future, it doesn't mean we should leave it in a broken state in the meantime. WHILE all of this is likely to be deleted, it has value until we delete it.

I'm not opposed to moving forward with this check, I just didn't want @yihanaa (and reviewers) to spend time working on this if it's going to be removed in this release of Clang anyway.

clang/test/CodeGen/dump-struct-builtin.c
810

You should add the newline back to the end of the test.

erichkeane accepted this revision.Apr 28 2022, 6:23 AM

CGBuiltin.cpp changes look good enough to me, but please add the newline aaron requested.

This revision is now accepted and ready to land.Apr 28 2022, 6:23 AM
yihanaa updated this revision to Diff 425861.Apr 28 2022, 11:35 AM

Add the newline back to the end of the test.

yihanaa marked an inline comment as done.EditedApr 28 2022, 11:37 AM

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

This revision was landed with ongoing or failed builds.Apr 28 2022, 10:22 PM
This revision was automatically updated to reflect the committed changes.
yihanaa added a comment.EditedApr 28 2022, 10:30 PM

Thank you all, I have commit in eaca933c59fd61b3df4697b5fae0eeec67acfaeb