draft: implement .include directive in sbpf compiler#192
draft: implement .include directive in sbpf compiler#192datasalaryman wants to merge 1 commit intoanza-xyz:solana-rustc/20.1-2025-02-13from
Conversation
|
Given that users of this toolchain would use |
Not a comment on the implementation, but if the Anza toolchain would like to remain a first class citizen for its own assembly dialect instead of falling further behind sbpf, it makes sense to implement what assembly users actually want, even if C preprocessing is a reasonable alternative. |
nagisa
left a comment
There was a problem hiding this comment.
LLVM tooling already supports .include for the sbf-* targets and given that .include is pretty much just splicing the referenced file in the specific location .globl/.global limitation seems unnecessary.
Please provide proper explanation for why these changes are being made, cause as is, it is not doing what it says on the tin.
|
|
||
| /// Parse .globl or .global directive for SBF. | ||
| /// SBF requires that .globl/.global only be used in the main file, not in | ||
| /// files that are included via .include directive. |
There was a problem hiding this comment.
Thank you for your comments! It's been a long while since I've read and written in C/C++.
Conceptually, it makes sense to drop the .globl on the included files so that we preserve the idea to the .global entrypoint on the main.s. Makes it easier to rememeber.
This is still a draft. I'm dropping the reimplementation and am instead writing additional tests to make sure the .include directive will compile successfully.
There was a problem hiding this comment.
Conceptually, it makes sense to drop the .globl on the included files so that we preserve the idea to the .global entrypoint on the main.s. Makes it easier to rememeber.
I suspect that your understanding of what .globl does might be incorrect. Even then I don't see any reason why the assembler would be responsible for preventing users from exporting the entrypoint symbol from elsewhere than the primary assembly file.
I could imagine a restriction along the lines of only one exported symbol, but assembler is not really the correct layer for this logic either.
51d62ff to
f558067
Compare
|
@nagisa - just force-pushed new changes For now, I've added additional tests parallel to the sbpf repo so we can check that both work similarly. I've included a test that will fail if we don't enforce the .globl constraint, which we can continue to discuss. |
f558067 to
bac4559
Compare
@blueshift-gg is implementing the .include directive for sBPF here - blueshift-gg/sbpf#109. propsing this change for backwards compatibility.
Vibe-coded this implementation and am posting this as a draft for now. Will make a proper contribution once I inspect the diff well and understand what actually needs to be shipped.
Any feedback on this change and pointers for this would be welcome.