[ClangIR][CIRGen] Handle nested union in arrays of struct #1007
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Just a heads up: I usually don't review patches failing to pass tests, let me know when this is ready |
bcardosolopes
changed the title
|
(Triggering rerunning) |
|
@bcardosolopes the CI is green now |
|
I'm working on adding an |
Yes, this was the reason I called it as a |
|
I think one motivating example may be, if in some transformation we changed the init of a union into a undef, then it looks slightly prettier: than at least we know the expected initialized field. |
|
Yeah, that makes sense to me, though I'll wait for Bruno to weigh in too :) |
bcardosolopes
requested changes
Oct 29, 2024
bcardosolopes
requested changes
Oct 30, 2024
|
looks like new CI failures! |
ChuanqiXu9
force-pushed
the
handling_union_in_array_struct
branch
from
949f67d to
55374fa
Compare
|
It looks like the windows failure is not relevant. |
bcardosolopes
approved these changes
Nov 4, 2024
This was referenced Nov 25, 2024
bcardosolopes
pushed a commit
that referenced
this pull request
Nov 27, 2024
…1166) Close #1131 This is another solution to #1160 This patch revert #1007 and remain its test. The problem described in #1007 is workaround by skipping the check of equivalent of element types in arrays. We can't mock such checks simply by adding another attribute to `ConstStructAttr` since the types are aggregated. e.g., we have to handle the cases like `struct { union { ... } }` and `struct { struct { union { ... } } }` and so on. To make it, we have to introduce what I called "two type systems" in #1160. This is not very good giving it removes a reasonable check. But it might not be so problematic since the Sema part has already checked it. (Of course, we still need face the risks to introduce new bugs any way)
This was referenced Nov 28, 2024
lanza
pushed a commit
that referenced
this pull request
Mar 18, 2025
Reproducer:
```
struct nested
{
union {
const char *single;
const char *const *multi;
} output;
};
static const char * const test[] = {
"test",
};
const struct nested data[] =
{
{
{
.multi = test,
},
},
{
{
.single = "hello",
},
},
};
```
ClangIR now failed to recognize `data` as an array since it failed to
recognize the initializer for union. This comes from a fundamental
difference between CIR and LLVM IR. In LLVM IR, the union is simply a
struct with the largest member. So it is fine to have only one init
element. But in CIR, the union has the information for all members. So
if we only pass a single init element, we may be in trouble. We solve
the problem by appending placeholder attribute for the uninitialized
fields.
lanza
pushed a commit
that referenced
this pull request
Mar 18, 2025
…1166) Close #1131 This is another solution to #1160 This patch revert #1007 and remain its test. The problem described in #1007 is workaround by skipping the check of equivalent of element types in arrays. We can't mock such checks simply by adding another attribute to `ConstStructAttr` since the types are aggregated. e.g., we have to handle the cases like `struct { union { ... } }` and `struct { struct { union { ... } } }` and so on. To make it, we have to introduce what I called "two type systems" in #1160. This is not very good giving it removes a reasonable check. But it might not be so problematic since the Sema part has already checked it. (Of course, we still need face the risks to introduce new bugs any way)
xlauko
pushed a commit
to trailofbits/instafix-llvm
that referenced
this pull request
Mar 28, 2025
…lvm#1166) Close llvm/clangir#1131 This is another solution to llvm/clangir#1160 This patch revert llvm/clangir#1007 and remain its test. The problem described in llvm/clangir#1007 is workaround by skipping the check of equivalent of element types in arrays. We can't mock such checks simply by adding another attribute to `ConstStructAttr` since the types are aggregated. e.g., we have to handle the cases like `struct { union { ... } }` and `struct { struct { union { ... } } }` and so on. To make it, we have to introduce what I called "two type systems" in llvm/clangir#1160. This is not very good giving it removes a reasonable check. But it might not be so problematic since the Sema part has already checked it. (Of course, we still need face the risks to introduce new bugs any way)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reproducer:
ClangIR now failed to recognize
dataas an array since it failed to recognize the initializer for union. This comes from a fundamental difference between CIR and LLVM IR. In LLVM IR, the union is simply a struct with the largest member. So it is fine to have only one init element. But in CIR, the union has the information for all members. So if we only pass a single init element, we may be in trouble. We solve the problem by appending placeholder attribute for the uninitialized fields.