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

gen_digestof_box uses wrong type for phi expression #2683

Closed
jasonbking opened this issue May 2, 2018 · 4 comments
Closed

gen_digestof_box uses wrong type for phi expression #2683

jasonbking opened this issue May 2, 2018 · 4 comments

Comments

@jasonbking
Copy link
Contributor

While porting pony to illumos, I kept triggering an LLVM assertion 'Assertion failed: getType() == V->getType() && "All operands to PHI node must be the same type as the PHI node!" on LLVM 3.9.1 (on LLVM 5.0 and 6.0 I'd get either strange libponyc test failures or segfaults). The problem appears to be the following line from src/libponyc/codegen/genreference.c:377

LLVMValueRef phi = LLVMBuildPhi(c->builder, c->i64, "");

All the other __digestof functions created appear to return the type contained in c->intptr, which for a 32-binary is i32, not i64 (adding some print statements prior to the assertion confirm this). It seems like the above line should also use c->intptr so the correct type is used for both 32 and 64-bit.

@ikavalio
Copy link
Contributor

ikavalio commented May 4, 2018

It happens to me on x86 and 32bit arm (linux) as well, so I believe @jasonbking is correct and c->intptr resolves the problem.

@jasonbking
Copy link
Contributor Author

That makes me feel better that I've found a legitimate bug, and not just a platform quirk :). So we know there's a potential mismatch in return types of __digestof functions on 32-bit platforms. Since all of the other ones except this one use c->intptr to obtain the return type, my assumption is that this one should as well. However, before I submit a PR, it'd be nice if someone familiar with the code and the intent here could confirm things when they have an opportunity -- it could be possible it's backwards and everything should be using c->i64.

@Praetonus
Copy link
Member

This is an oversight from #2615, sorry for that. Replacing i64 with intptr here is indeed the correct fix.

@mfelsche
Copy link
Contributor

mfelsche commented May 9, 2018

This is fixed by merging #2691

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

No branches or pull requests

5 participants