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][IR] Relax get_member verifier for incomplete types #269

Merged
merged 8 commits into from
Oct 18, 2023

Conversation

gitoleg
Copy link
Collaborator

@gitoleg gitoleg commented Sep 23, 2023

This is a suggestion to relax the existing verification even more than we did it in PR #257.
Here we also skip verification if a field on the given index is also of incomplete type - and we can not compare it with the result type of the operation.

Now the next code fails with type mismatch error:

typedef struct Node {    
    struct Node* next;
} NodeStru;

void foo(NodeStru* a) {        
    a->next = 0;
}

because the result type is kind of full and the type of field is not (for the reasons discussed in #256).
Basically, the problem is in the GetMemberOp result type generated as following (via CIRGenTypes::convertType)
!cir.ptr<!cir.struct<struct "Node" {!cir.ptr<!cir.struct<struct "Node" incomplete #cir.record.decl.ast>>} #cir.record.decl.ast>>

where the field type at index differs from the record type - compare with
!cir.ptr<!cir.struct<struct "Node" incomplete #cir.record.decl.ast>>

We just slightly relax the previous solution in #257 - and the compilation won't fail in the case of recursive types.

Well, if there are some other thoughts?

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.

While @sitio-couto works on the proper codegen fix, this should be good. Please add a testcase and fix comments below.

clang/lib/CIR/Dialect/IR/CIRDialect.cpp Outdated Show resolved Hide resolved
clang/lib/CIR/Dialect/IR/CIRDialect.cpp Outdated Show resolved Hide resolved
@gitoleg
Copy link
Collaborator Author

gitoleg commented Sep 26, 2023

@bcardosolopes done!

Added a small test - I think it will fail once the issue with recursive types will be fixed, and thus we'll remember to restore type checking for GetMemberOp

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.

One piece left!

clang/test/CIR/CodeGen/struct.c Outdated Show resolved Hide resolved
clang/test/CIR/CodeGen/struct.c Outdated Show resolved Hide resolved
@gitoleg
Copy link
Collaborator Author

gitoleg commented Sep 27, 2023

@bcardosolopes done

@gitoleg
Copy link
Collaborator Author

gitoleg commented Oct 11, 2023

@bcardosolopes @sitio-couto
So, how far are we from the recursive types support? Do we need this PR? Once we are near, I can close it. Otherwise, let's merge? Or there are some doubts ?

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

@bcardosolopes bcardosolopes merged commit 37aa0f2 into llvm:main Oct 18, 2023
lanza pushed a commit that referenced this pull request Oct 25, 2023
This is a suggestion to relax the existing verification even more than
we did it in PR #257.
Here we also skip verification if a field on the given index is also of
incomplete type - and we can not compare it with the result type of the
operation.

Now the next code fails with type mismatch error:
```
typedef struct Node {    
    struct Node* next;
} NodeStru;

void foo(NodeStru* a) {        
    a->next = 0;
}
```
because the result type is kind of full and the type of field is not
(for the reasons discussed in #256).
Basically, the problem is in the `GetMemberOp` result type generated as
following (via `CIRGenTypes::convertType`)
`!cir.ptr<!cir.struct<struct "Node" {!cir.ptr<!cir.struct<struct "Node"
incomplete #cir.record.decl.ast>>} #cir.record.decl.ast>>`

where the field type at index differs from the record type - compare
with
 `!cir.ptr<!cir.struct<struct "Node" incomplete #cir.record.decl.ast>>`

We just slightly relax the previous solution in #257 - and the
compilation won't fail in the case of recursive types.

Well, if there are some other thoughts?
lanza pushed a commit that referenced this pull request Oct 27, 2023
This is a suggestion to relax the existing verification even more than
we did it in PR #257.
Here we also skip verification if a field on the given index is also of
incomplete type - and we can not compare it with the result type of the
operation.

Now the next code fails with type mismatch error:
```
typedef struct Node {    
    struct Node* next;
} NodeStru;

void foo(NodeStru* a) {        
    a->next = 0;
}
```
because the result type is kind of full and the type of field is not
(for the reasons discussed in #256).
Basically, the problem is in the `GetMemberOp` result type generated as
following (via `CIRGenTypes::convertType`)
`!cir.ptr<!cir.struct<struct "Node" {!cir.ptr<!cir.struct<struct "Node"
incomplete #cir.record.decl.ast>>} #cir.record.decl.ast>>`

where the field type at index differs from the record type - compare
with
 `!cir.ptr<!cir.struct<struct "Node" incomplete #cir.record.decl.ast>>`

We just slightly relax the previous solution in #257 - and the
compilation won't fail in the case of recursive types.

Well, if there are some other thoughts?
lanza pushed a commit that referenced this pull request Dec 20, 2023
This is a suggestion to relax the existing verification even more than
we did it in PR #257.
Here we also skip verification if a field on the given index is also of
incomplete type - and we can not compare it with the result type of the
operation.

Now the next code fails with type mismatch error:
```
typedef struct Node {    
    struct Node* next;
} NodeStru;

void foo(NodeStru* a) {        
    a->next = 0;
}
```
because the result type is kind of full and the type of field is not
(for the reasons discussed in #256).
Basically, the problem is in the `GetMemberOp` result type generated as
following (via `CIRGenTypes::convertType`)
`!cir.ptr<!cir.struct<struct "Node" {!cir.ptr<!cir.struct<struct "Node"
incomplete #cir.record.decl.ast>>} #cir.record.decl.ast>>`

where the field type at index differs from the record type - compare
with
 `!cir.ptr<!cir.struct<struct "Node" incomplete #cir.record.decl.ast>>`

We just slightly relax the previous solution in #257 - and the
compilation won't fail in the case of recursive types.

Well, if there are some other thoughts?
lanza pushed a commit that referenced this pull request Jan 29, 2024
This is a suggestion to relax the existing verification even more than
we did it in PR #257.
Here we also skip verification if a field on the given index is also of
incomplete type - and we can not compare it with the result type of the
operation.

Now the next code fails with type mismatch error:
```
typedef struct Node {    
    struct Node* next;
} NodeStru;

void foo(NodeStru* a) {        
    a->next = 0;
}
```
because the result type is kind of full and the type of field is not
(for the reasons discussed in #256).
Basically, the problem is in the `GetMemberOp` result type generated as
following (via `CIRGenTypes::convertType`)
`!cir.ptr<!cir.struct<struct "Node" {!cir.ptr<!cir.struct<struct "Node"
incomplete #cir.record.decl.ast>>} #cir.record.decl.ast>>`

where the field type at index differs from the record type - compare
with
 `!cir.ptr<!cir.struct<struct "Node" incomplete #cir.record.decl.ast>>`

We just slightly relax the previous solution in #257 - and the
compilation won't fail in the case of recursive types.

Well, if there are some other thoughts?
lanza pushed a commit that referenced this pull request Mar 23, 2024
This is a suggestion to relax the existing verification even more than
we did it in PR #257.
Here we also skip verification if a field on the given index is also of
incomplete type - and we can not compare it with the result type of the
operation.

Now the next code fails with type mismatch error:
```
typedef struct Node {    
    struct Node* next;
} NodeStru;

void foo(NodeStru* a) {        
    a->next = 0;
}
```
because the result type is kind of full and the type of field is not
(for the reasons discussed in #256).
Basically, the problem is in the `GetMemberOp` result type generated as
following (via `CIRGenTypes::convertType`)
`!cir.ptr<!cir.struct<struct "Node" {!cir.ptr<!cir.struct<struct "Node"
incomplete #cir.record.decl.ast>>} #cir.record.decl.ast>>`

where the field type at index differs from the record type - compare
with
 `!cir.ptr<!cir.struct<struct "Node" incomplete #cir.record.decl.ast>>`

We just slightly relax the previous solution in #257 - and the
compilation won't fail in the case of recursive types.

Well, if there are some other thoughts?
eZWALT pushed a commit to eZWALT/clangir that referenced this pull request Mar 24, 2024
This is a suggestion to relax the existing verification even more than
we did it in PR llvm#257.
Here we also skip verification if a field on the given index is also of
incomplete type - and we can not compare it with the result type of the
operation.

Now the next code fails with type mismatch error:
```
typedef struct Node {    
    struct Node* next;
} NodeStru;

void foo(NodeStru* a) {        
    a->next = 0;
}
```
because the result type is kind of full and the type of field is not
(for the reasons discussed in llvm#256).
Basically, the problem is in the `GetMemberOp` result type generated as
following (via `CIRGenTypes::convertType`)
`!cir.ptr<!cir.struct<struct "Node" {!cir.ptr<!cir.struct<struct "Node"
incomplete #cir.record.decl.ast>>} #cir.record.decl.ast>>`

where the field type at index differs from the record type - compare
with
 `!cir.ptr<!cir.struct<struct "Node" incomplete #cir.record.decl.ast>>`

We just slightly relax the previous solution in llvm#257 - and the
compilation won't fail in the case of recursive types.

Well, if there are some other thoughts?
eZWALT pushed a commit to eZWALT/clangir that referenced this pull request Mar 24, 2024
This is a suggestion to relax the existing verification even more than
we did it in PR llvm#257.
Here we also skip verification if a field on the given index is also of
incomplete type - and we can not compare it with the result type of the
operation.

Now the next code fails with type mismatch error:
```
typedef struct Node {    
    struct Node* next;
} NodeStru;

void foo(NodeStru* a) {        
    a->next = 0;
}
```
because the result type is kind of full and the type of field is not
(for the reasons discussed in llvm#256).
Basically, the problem is in the `GetMemberOp` result type generated as
following (via `CIRGenTypes::convertType`)
`!cir.ptr<!cir.struct<struct "Node" {!cir.ptr<!cir.struct<struct "Node"
incomplete #cir.record.decl.ast>>} #cir.record.decl.ast>>`

where the field type at index differs from the record type - compare
with
 `!cir.ptr<!cir.struct<struct "Node" incomplete #cir.record.decl.ast>>`

We just slightly relax the previous solution in llvm#257 - and the
compilation won't fail in the case of recursive types.

Well, if there are some other thoughts?
lanza pushed a commit that referenced this pull request Apr 29, 2024
This is a suggestion to relax the existing verification even more than
we did it in PR #257.
Here we also skip verification if a field on the given index is also of
incomplete type - and we can not compare it with the result type of the
operation.

Now the next code fails with type mismatch error:
```
typedef struct Node {    
    struct Node* next;
} NodeStru;

void foo(NodeStru* a) {        
    a->next = 0;
}
```
because the result type is kind of full and the type of field is not
(for the reasons discussed in #256).
Basically, the problem is in the `GetMemberOp` result type generated as
following (via `CIRGenTypes::convertType`)
`!cir.ptr<!cir.struct<struct "Node" {!cir.ptr<!cir.struct<struct "Node"
incomplete #cir.record.decl.ast>>} #cir.record.decl.ast>>`

where the field type at index differs from the record type - compare
with
 `!cir.ptr<!cir.struct<struct "Node" incomplete #cir.record.decl.ast>>`

We just slightly relax the previous solution in #257 - and the
compilation won't fail in the case of recursive types.

Well, if there are some other thoughts?
lanza pushed a commit that referenced this pull request Apr 29, 2024
This is a suggestion to relax the existing verification even more than
we did it in PR #257.
Here we also skip verification if a field on the given index is also of
incomplete type - and we can not compare it with the result type of the
operation.

Now the next code fails with type mismatch error:
```
typedef struct Node {    
    struct Node* next;
} NodeStru;

void foo(NodeStru* a) {        
    a->next = 0;
}
```
because the result type is kind of full and the type of field is not
(for the reasons discussed in #256).
Basically, the problem is in the `GetMemberOp` result type generated as
following (via `CIRGenTypes::convertType`)
`!cir.ptr<!cir.struct<struct "Node" {!cir.ptr<!cir.struct<struct "Node"
incomplete #cir.record.decl.ast>>} #cir.record.decl.ast>>`

where the field type at index differs from the record type - compare
with
 `!cir.ptr<!cir.struct<struct "Node" incomplete #cir.record.decl.ast>>`

We just slightly relax the previous solution in #257 - and the
compilation won't fail in the case of recursive types.

Well, if there are some other thoughts?
eZWALT pushed a commit to eZWALT/clangir that referenced this pull request Apr 29, 2024
This is a suggestion to relax the existing verification even more than
we did it in PR llvm#257.
Here we also skip verification if a field on the given index is also of
incomplete type - and we can not compare it with the result type of the
operation.

Now the next code fails with type mismatch error:
```
typedef struct Node {    
    struct Node* next;
} NodeStru;

void foo(NodeStru* a) {        
    a->next = 0;
}
```
because the result type is kind of full and the type of field is not
(for the reasons discussed in llvm#256).
Basically, the problem is in the `GetMemberOp` result type generated as
following (via `CIRGenTypes::convertType`)
`!cir.ptr<!cir.struct<struct "Node" {!cir.ptr<!cir.struct<struct "Node"
incomplete #cir.record.decl.ast>>} #cir.record.decl.ast>>`

where the field type at index differs from the record type - compare
with
 `!cir.ptr<!cir.struct<struct "Node" incomplete #cir.record.decl.ast>>`

We just slightly relax the previous solution in llvm#257 - and the
compilation won't fail in the case of recursive types.

Well, if there are some other thoughts?
lanza pushed a commit that referenced this pull request Apr 29, 2024
This is a suggestion to relax the existing verification even more than
we did it in PR #257.
Here we also skip verification if a field on the given index is also of
incomplete type - and we can not compare it with the result type of the
operation.

Now the next code fails with type mismatch error:
```
typedef struct Node {
    struct Node* next;
} NodeStru;

void foo(NodeStru* a) {
    a->next = 0;
}
```
because the result type is kind of full and the type of field is not
(for the reasons discussed in #256).
Basically, the problem is in the `GetMemberOp` result type generated as
following (via `CIRGenTypes::convertType`)
`!cir.ptr<!cir.struct<struct "Node" {!cir.ptr<!cir.struct<struct "Node"
incomplete #cir.record.decl.ast>>} #cir.record.decl.ast>>`

where the field type at index differs from the record type - compare
with
 `!cir.ptr<!cir.struct<struct "Node" incomplete #cir.record.decl.ast>>`

We just slightly relax the previous solution in #257 - and the
compilation won't fail in the case of recursive types.

Well, if there are some other thoughts?
bruteforceboy pushed a commit to bruteforceboy/clangir that referenced this pull request Oct 2, 2024
This is a suggestion to relax the existing verification even more than
we did it in PR llvm#257.
Here we also skip verification if a field on the given index is also of
incomplete type - and we can not compare it with the result type of the
operation.

Now the next code fails with type mismatch error:
```
typedef struct Node {
    struct Node* next;
} NodeStru;

void foo(NodeStru* a) {
    a->next = 0;
}
```
because the result type is kind of full and the type of field is not
(for the reasons discussed in llvm#256).
Basically, the problem is in the `GetMemberOp` result type generated as
following (via `CIRGenTypes::convertType`)
`!cir.ptr<!cir.struct<struct "Node" {!cir.ptr<!cir.struct<struct "Node"
incomplete #cir.record.decl.ast>>} #cir.record.decl.ast>>`

where the field type at index differs from the record type - compare
with
 `!cir.ptr<!cir.struct<struct "Node" incomplete #cir.record.decl.ast>>`

We just slightly relax the previous solution in llvm#257 - and the
compilation won't fail in the case of recursive types.

Well, if there are some other thoughts?
Hugobros3 pushed a commit to shady-gang/clangir that referenced this pull request Oct 2, 2024
This is a suggestion to relax the existing verification even more than
we did it in PR llvm#257.
Here we also skip verification if a field on the given index is also of
incomplete type - and we can not compare it with the result type of the
operation.

Now the next code fails with type mismatch error:
```
typedef struct Node {
    struct Node* next;
} NodeStru;

void foo(NodeStru* a) {
    a->next = 0;
}
```
because the result type is kind of full and the type of field is not
(for the reasons discussed in llvm#256).
Basically, the problem is in the `GetMemberOp` result type generated as
following (via `CIRGenTypes::convertType`)
`!cir.ptr<!cir.struct<struct "Node" {!cir.ptr<!cir.struct<struct "Node"
incomplete #cir.record.decl.ast>>} #cir.record.decl.ast>>`

where the field type at index differs from the record type - compare
with
 `!cir.ptr<!cir.struct<struct "Node" incomplete #cir.record.decl.ast>>`

We just slightly relax the previous solution in llvm#257 - and the
compilation won't fail in the case of recursive types.

Well, if there are some other thoughts?
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