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

[CIR][CodeGen] Fix packed structures #839

Merged
merged 5 commits into from
Sep 17, 2024

Conversation

bruteforceboy
Copy link
Contributor

@bruteforceboy bruteforceboy commented Sep 13, 2024

Consider the following code snippet test.c:

#pragma pack(2)

typedef struct {
    char b;
    int c;
} D;

typedef struct {
    D e;
    int f;
} E;

void f1() {
    E a = {};
}

When emitting the CIR using bin/clang test.c -Xclang -fclangir -Xclang -emit-cir -S -o - the current implementation gets:

NYI
UNREACHABLE executed at ~/clangir/clang/lib/CIR/CodeGen/CIRGenExprConst.cpp:338!

This is only one of the many tests where this happens.

Comparing the implementations of CIR/CodeGen/CIRRecordLayoutBuilder.cpp and clang's codegen lib/CodeGen/CGRecordLayoutBuilder.cpp, there is some padding missing for packed structures, and some alignments that need to be corrected.

This PR also updates 2 existing tests. In the first test, structural-binding.cpp, I updated some cir.get_member indexes. In the second test, packed-structs.c, I updated the cir layout for the structure, and added more tests. I have compared the changes I made in the tests to the original clang codegen and everything seems fine.

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for fixing this! One minor improvement before I merge:

I have compared the changes I made in the tests to the original clang codegen and everything seems fine.

Because you are looking at the LLVM IR output, it might be worth adding some testing for it to make sure we don't regress in case something changes in CIR. Can you add some of the key pieces you looked for when checking the LLVM output? clang/test/CIR/CodeGen/abstract-cond.c is an example how to do that, you could add an extra RUN line to packed-structs.c or create a new file.

@bruteforceboy
Copy link
Contributor Author

LGTM, thanks for fixing this! One minor improvement before I merge:

I have compared the changes I made in the tests to the original clang codegen and everything seems fine.

Because you are looking at the LLVM IR output, it might be worth adding some testing for it to make sure we don't regress in case something changes in CIR. Can you add some of the key pieces you looked for when checking the LLVM output? clang/test/CIR/CodeGen/abstract-cond.c is an example how to do that, you could add an extra RUN line to packed-structs.c or create a new file.

Sorry, I've been a busy the past few days. I have updated packed-structs.c, let me know if everything is okay.

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you!

@bcardosolopes bcardosolopes merged commit cdca0aa into llvm:main Sep 17, 2024
5 of 6 checks passed
Hugobros3 pushed a commit to shady-gang/clangir that referenced this pull request Oct 2, 2024
Consider the following code snippet `test.c`:
```
#pragma pack(2)

typedef struct {
    char b;
    int c;
} D;

typedef struct {
    D e;
    int f;
} E;

void f1() {
    E a = {};
}
```
When emitting the CIR using `bin/clang test.c -Xclang -fclangir -Xclang
-emit-cir -S -o -` the current implementation gets:
```
NYI
UNREACHABLE executed at ~/clangir/clang/lib/CIR/CodeGen/CIRGenExprConst.cpp:338!
 ```
This is only one of the many tests where this happens. 

Comparing the implementations of `CIR/CodeGen/CIRRecordLayoutBuilder.cpp` and clang's codegen `lib/CodeGen/CGRecordLayoutBuilder.cpp`, there is some padding missing for packed structures, and some alignments that need to be corrected. 

This PR also updates 2 existing tests. In the first test, `structural-binding.cpp`, I updated some `cir.get_member` indexes. In the second test, `packed-structs.c`, I updated the `cir` layout for the structure, and added more tests. I have compared the changes I made in the tests to the original clang codegen and everything seems fine.
smeenai pushed a commit to smeenai/clangir that referenced this pull request Oct 9, 2024
Consider the following code snippet `test.c`:
```

typedef struct {
    char b;
    int c;
} D;

typedef struct {
    D e;
    int f;
} E;

void f1() {
    E a = {};
}
```
When emitting the CIR using `bin/clang test.c -Xclang -fclangir -Xclang
-emit-cir -S -o -` the current implementation gets:
```
NYI
UNREACHABLE executed at ~/clangir/clang/lib/CIR/CodeGen/CIRGenExprConst.cpp:338!
 ```
This is only one of the many tests where this happens.

Comparing the implementations of `CIR/CodeGen/CIRRecordLayoutBuilder.cpp` and clang's codegen `lib/CodeGen/CGRecordLayoutBuilder.cpp`, there is some padding missing for packed structures, and some alignments that need to be corrected.

This PR also updates 2 existing tests. In the first test, `structural-binding.cpp`, I updated some `cir.get_member` indexes. In the second test, `packed-structs.c`, I updated the `cir` layout for the structure, and added more tests. I have compared the changes I made in the tests to the original clang codegen and everything seems fine.
smeenai pushed a commit to smeenai/clangir that referenced this pull request Oct 9, 2024
Consider the following code snippet `test.c`:
```

typedef struct {
    char b;
    int c;
} D;

typedef struct {
    D e;
    int f;
} E;

void f1() {
    E a = {};
}
```
When emitting the CIR using `bin/clang test.c -Xclang -fclangir -Xclang
-emit-cir -S -o -` the current implementation gets:
```
NYI
UNREACHABLE executed at ~/clangir/clang/lib/CIR/CodeGen/CIRGenExprConst.cpp:338!
 ```
This is only one of the many tests where this happens.

Comparing the implementations of `CIR/CodeGen/CIRRecordLayoutBuilder.cpp` and clang's codegen `lib/CodeGen/CGRecordLayoutBuilder.cpp`, there is some padding missing for packed structures, and some alignments that need to be corrected.

This PR also updates 2 existing tests. In the first test, `structural-binding.cpp`, I updated some `cir.get_member` indexes. In the second test, `packed-structs.c`, I updated the `cir` layout for the structure, and added more tests. I have compared the changes I made in the tests to the original clang codegen and everything seems fine.
smeenai pushed a commit to smeenai/clangir that referenced this pull request Oct 9, 2024
Consider the following code snippet `test.c`:
```

typedef struct {
    char b;
    int c;
} D;

typedef struct {
    D e;
    int f;
} E;

void f1() {
    E a = {};
}
```
When emitting the CIR using `bin/clang test.c -Xclang -fclangir -Xclang
-emit-cir -S -o -` the current implementation gets:
```
NYI
UNREACHABLE executed at ~/clangir/clang/lib/CIR/CodeGen/CIRGenExprConst.cpp:338!
 ```
This is only one of the many tests where this happens.

Comparing the implementations of `CIR/CodeGen/CIRRecordLayoutBuilder.cpp` and clang's codegen `lib/CodeGen/CGRecordLayoutBuilder.cpp`, there is some padding missing for packed structures, and some alignments that need to be corrected.

This PR also updates 2 existing tests. In the first test, `structural-binding.cpp`, I updated some `cir.get_member` indexes. In the second test, `packed-structs.c`, I updated the `cir` layout for the structure, and added more tests. I have compared the changes I made in the tests to the original clang codegen and everything seems fine.
keryell pushed a commit to keryell/clangir that referenced this pull request Oct 19, 2024
Consider the following code snippet `test.c`:
```

typedef struct {
    char b;
    int c;
} D;

typedef struct {
    D e;
    int f;
} E;

void f1() {
    E a = {};
}
```
When emitting the CIR using `bin/clang test.c -Xclang -fclangir -Xclang
-emit-cir -S -o -` the current implementation gets:
```
NYI
UNREACHABLE executed at ~/clangir/clang/lib/CIR/CodeGen/CIRGenExprConst.cpp:338!
 ```
This is only one of the many tests where this happens.

Comparing the implementations of `CIR/CodeGen/CIRRecordLayoutBuilder.cpp` and clang's codegen `lib/CodeGen/CGRecordLayoutBuilder.cpp`, there is some padding missing for packed structures, and some alignments that need to be corrected.

This PR also updates 2 existing tests. In the first test, `structural-binding.cpp`, I updated some `cir.get_member` indexes. In the second test, `packed-structs.c`, I updated the `cir` layout for the structure, and added more tests. I have compared the changes I made in the tests to the original clang codegen and everything seems fine.
lanza pushed a commit that referenced this pull request Nov 5, 2024
Consider the following code snippet `test.c`:
```

typedef struct {
    char b;
    int c;
} D;

typedef struct {
    D e;
    int f;
} E;

void f1() {
    E a = {};
}
```
When emitting the CIR using `bin/clang test.c -Xclang -fclangir -Xclang
-emit-cir -S -o -` the current implementation gets:
```
NYI
UNREACHABLE executed at ~/clangir/clang/lib/CIR/CodeGen/CIRGenExprConst.cpp:338!
 ```
This is only one of the many tests where this happens.

Comparing the implementations of `CIR/CodeGen/CIRRecordLayoutBuilder.cpp` and clang's codegen `lib/CodeGen/CGRecordLayoutBuilder.cpp`, there is some padding missing for packed structures, and some alignments that need to be corrected.

This PR also updates 2 existing tests. In the first test, `structural-binding.cpp`, I updated some `cir.get_member` indexes. In the second test, `packed-structs.c`, I updated the `cir` layout for the structure, and added more tests. I have compared the changes I made in the tests to the original clang codegen and everything seems fine.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants