Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Union Initializer Fails During CIRGen #1131

Closed
bruteforceboy opened this issue Nov 15, 2024 · 3 comments · Fixed by #1166
Closed

Union Initializer Fails During CIRGen #1131

bruteforceboy opened this issue Nov 15, 2024 · 3 comments · Fixed by #1166
Assignees

Comments

@bruteforceboy
Copy link
Contributor

bruteforceboy commented Nov 15, 2024

The following code snippet fails when generating CIR, and that didn't happen until PR#1007 was merged.

typedef struct {
  union {
    int a;
    long b;
  };
} S;

S s = { .a = 1 };

It seems there is some extra padding even after unpacking the union, and there is an error when computing the size?

I tried to fix it myself, but then it fails again when lowering to LLVM, because b is marked as inactive, and a is the "active" field, but when lowering to LLVM, the type converter uses the larger member of the union.

cc: @bcardosolopes and @ChuanqiXu9 since you authored the PR)

@ChuanqiXu9
Copy link
Member

Maybe we shouldn't go into

} else if (DesiredSize > AlignedSize) {
// The natural layout would be too small. Add padding to fix it. (This
// is ignored if we choose a packed layout.)
UnpackedElemStorage.assign(UnpackedElems.begin(), UnpackedElems.end());
UnpackedElemStorage.push_back(Utils.getPadding(DesiredSize - Size));
UnpackedElems = UnpackedElemStorage;
}
when we're making union in the above branch?

I'd like to look into the details a few days later.

@ChuanqiXu9 ChuanqiXu9 self-assigned this Nov 21, 2024
@bcardosolopes
Copy link
Member

cc @gitoleg

@ChuanqiXu9
Copy link
Member

Sent #1160

bcardosolopes pushed a commit that referenced this issue 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants