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

Add support for opaque pointers and structs #51

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rj45
Copy link
Contributor

@rj45 rj45 commented Aug 27, 2023

If you call ElementType() on an opaque pointer, it causes a segfault. Unfortunately there was no way to know you had an opaque pointer on your hands before this change, so there was no way to know that calling ElementType() on the type that was a PointerTypeKind would crash.

This change introduces a IsPointerOpaque() method to check for opaque pointers. While I was at it, I noticed there was no way to check for opaque structs as well, so I added that. I also tried calling Subtypes() and noticed it causes an index out of range panic when there is no subtypes, so I fixed that too.

When I uploaded the fix, I noticed that LLVM 14 doesn't have the method to check for opaque pointers, which is odd because opaque pointers have been a thing since 2015.... at any rate, it was simple enough to add a back port for it, though it took a couple tries to get the CI to pass.

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

Looks reasonable.

When I added support for LLVM 15 (with opaque pointer support) I didn't need this. I usually used all these helper methods like GlobalValueType and the like to replace ElementType calls, see: tinygo-org/tinygo@2fd669c. But if you're building a backend then I suppose this may be necessary.

@aykevl
Copy link
Member

aykevl commented Aug 29, 2023

I'm not sure what's up with the CI. I hope it's a temporary failure.

@deadprogram
Copy link
Member

To use the bundled libc++ please add the following LDFLAGS:
LDFLAGS="-L$HOMEBREW_PREFIX/opt/llvm/lib/c++ -Wl,-rpath,$HOMEBREW_PREFIX/opt/llvm/lib/c++"

Looks like this needs to be done to pass CI on macOS...

@rj45
Copy link
Contributor Author

rj45 commented Sep 2, 2023

Seemed like python was failing to install in the CI.... I tried a fix to force python to install, but now it's saying that python is already installed. So it must have been updated in the base image. So I reverted my "fix" since it shouldn't be necessary.

@aykevl
Copy link
Member

aykevl commented Oct 1, 2023

The fix for opaque structs seems necessary indeed (and is unrelated to opaque pointers: opaque structs are simply structs without a body and are part of LLVM for years).

I'm not so sure about specific support for opaque pointers. They were last used by default in LLVM 14, and have been removed entirely in LLVM 17. So I'd say that new code should assume all pointers are opaque. Or do you still need to compile against LLVM 14 for some reason?

This is what LLVMPointerTypeIsOpaque looks like in LLVM 17:

LLVMBool LLVMPointerTypeIsOpaque(LLVMTypeRef Ty) {
  return true;
}

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.

3 participants