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

structmemfunclowering: fix crash when gep is optimized away #203

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

Hyxogen
Copy link
Contributor

@Hyxogen Hyxogen commented Feb 6, 2024

LLVM is smart enough to be able to remove a gep if the source and indexes are constants. However, it was assumed that it would always emit a gep instruction, which would cause a crash when trying to cast it.

Upon further inspection, I believe that this casting to a GEPOperator and then getting the result element type is overkill, and we could just simply use the type of the value.

This fixes: leaningtech/cheerp-meta#141

@Hyxogen Hyxogen requested a review from yuri91 February 6, 2024 10:16
@yuri91
Copy link
Member

yuri91 commented Feb 6, 2024

llvm is removing getPointerElementType(), so maybe that's why we do the overkill cast there. At some point we started removing all the "easy" cases of getPointerElementType usage.

@yuri91
Copy link
Member

yuri91 commented Feb 6, 2024

What is the actual type that we are trying to cast?

@Hyxogen
Copy link
Contributor Author

Hyxogen commented Feb 6, 2024

In the example from issue from cheerp-meta, it was i16** null

@yuri91
Copy link
Member

yuri91 commented Feb 6, 2024

Ah I see that it's a completely degenerate case. Can we just skip applying the pass in this case?

@Hyxogen
Copy link
Contributor Author

Hyxogen commented Feb 6, 2024

I'll check, I also saw this commit which I believe was never merged, it might also solve it:
9e9e527

@yuri91
Copy link
Member

yuri91 commented Feb 6, 2024

or even assuming that the type is char* if it's not inferrable from a gep

LLVM is smart enough to be able to fold a gep if the source and indices
are constants. However, it was assumed that it would always emit a gep
instruction, which would cause a crash when trying to cast it.

Upon further inspection, I believe that the surrounding if statement was
meant as a way of working around one of such folds. The new code should
work for the general case, so I've removed it.

This fixes: leaningtech/cheerp-meta#141
@Hyxogen Hyxogen force-pushed the fix-recursivecopy-crash branch from f3e1901 to a1235f5 Compare February 6, 2024 12:58
@Hyxogen
Copy link
Contributor Author

Hyxogen commented Feb 6, 2024

I've removed the use of getPointerElementType. Also upon further inspection, I believe that the surrounding if statement was
meant as a way of working around one of such optimizations. The new code should work for the general case, so I've removed it.

@Hyxogen
Copy link
Contributor Author

Hyxogen commented Feb 6, 2024

@yuri91

Copy link
Member

@yuri91 yuri91 left a comment

Choose a reason for hiding this comment

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

If the original issue is fixed, close it saying that it's fixed in master

@yuri91 yuri91 merged commit 97ae7db into master Feb 6, 2024
1 check passed
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.

A crash bug due to recursiveCopy?
2 participants