Generate global variable accesses and function calls to go via the custom GOT#9
Open
lewis-revill wants to merge 2 commits intomasterfrom
Open
Generate global variable accesses and function calls to go via the custom GOT#9lewis-revill wants to merge 2 commits intomasterfrom
lewis-revill wants to merge 2 commits intomasterfrom
Conversation
CohenArthur
reviewed
Jun 19, 2024
src/codegen.rs
Outdated
| } | ||
|
|
||
| let s = | ||
| read_to_string(location).map_err(|_| Diagnostic::new("GOT layout could not be read from file"))?; |
Member
There was a problem hiding this comment.
I think it is clearer to keep the module when calling free-standing public functions like this one, so I would rather see this:
Suggested change
| read_to_string(location).map_err(|_| Diagnostic::new("GOT layout could not be read from file"))?; | |
| fs::read_to_string(location).map_err(|_| Diagnostic::new("GOT layout could not be read from file"))?; |
and remove the import for fs::{read_to_string, write} in favor of an import of fs. this is definitely a nit so feel free to ignore, just easier for me to read the code
src/codegen.rs
Outdated
Comment on lines
+186
to
+187
| PouIndexEntry::Function { name, linkage: LinkageType::Internal, is_generated: false, .. } | ||
| | PouIndexEntry::Function { name, linkage: LinkageType::Internal, .. } => Some(name.as_ref()), |
Member
There was a problem hiding this comment.
maybe I'm not thinking hard enough but isn't that or-pattern redundant? if we are filtering on all PouIndexEntry::Functions that have internal linkage and which have is_generated set to false, but also on all the PouIndexEntry::Functions where that last condition isn't necessarily upheld (and is by thus true), then I feel like the first part of the pattern isn't necessary?
Suggested change
| PouIndexEntry::Function { name, linkage: LinkageType::Internal, is_generated: false, .. } | |
| | PouIndexEntry::Function { name, linkage: LinkageType::Internal, .. } => Some(name.as_ref()), | |
| PouIndexEntry::Function { name, linkage: LinkageType::Internal, .. } => Some(name.as_ref()), |
Collaborator
Author
There was a problem hiding this comment.
Ah good catch the second line should be PouIndexEntry::FunctionBlock
| .context | ||
| .i64_type() | ||
| .const_int(0xdeadbeef00000000, false) | ||
| .const_to_pointer(llvm_type.ptr_type(0.into()).ptr_type(0.into())); |
Member
There was a problem hiding this comment.
Suggested change
| .const_to_pointer(llvm_type.ptr_type(0.into()).ptr_type(0.into())); | |
| .const_to_pointer(llvm_type.ptr_type(AddressSpace::default()) | |
| .ptr_type(AddressSpace::default())); |
nit, but maybe clearer?
This commit introduces the association of GOT indices to the LLVM index, which then allows us to utilise that when generating references to variables to check if a given variable has an entry in the GOT. If so, we obtain its index, and generate the necessary LLVM IR to access the address contained within the GOT rather than accessing the variable directly.
lewis-revill
force-pushed
the
ljr-custom-got-globals
branch
from
d706b10 to
e80995a
Compare
CohenArthur
approved these changes
Jun 20, 2024
| .context | ||
| .i64_type() | ||
| .const_int(0xdeadbeef00000000, false) | ||
| .const_to_pointer(llvm_type |
Member
There was a problem hiding this comment.
Suggested change
| .const_to_pointer(llvm_type | |
| .const_to_pointer(function_type |
typo?
src/codegen.rs
Outdated
Comment on lines
+120
to
+121
| fs::write(location, s).map_err(|_| Diagnostic::new("GOT layout could not be written to file"))?; | ||
| Ok(()) |
Member
There was a problem hiding this comment.
Suggested change
| fs::write(location, s).map_err(|_| Diagnostic::new("GOT layout could not be written to file"))?; | |
| Ok(()) | |
| fs::write(location, s).map_err(|_| Diagnostic::new("GOT layout could not be written to file")) |
this can be simplified a little bit since you map the error properly already
This change involves moving the generation of the GOT from variable_generator.rs to codegen.rs, in order to also cover not only global variables but also functions and 'programs' too. Once these have been given an associated index in the GOT we can use that to replace normal direct function calls with indirect calls to a function pointer stored in the GOT. We don't do this for calls with external linkage since these won't be subject to online change.
lewis-revill
force-pushed
the
ljr-custom-got-globals
branch
from
e80995a to
c9a08a9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These patches contain changes to replace normal global variable accesses and direct function calls with an additional level of indirection through the custom GOT. Until load time this custom GOT must just be represented as a magic constant.
We may not cover all the possible paths that generate global accesses, and there is slight duplication of code between
generate_got_accessandgenerate_got_call. We also will want to use a crate to provide us with the definition of the magic constant rather than hardcoding it.