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

Generate global variable accesses and function calls to go via the custom GOT #9

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

Conversation

lewis-revill
Copy link

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_access and generate_got_call. We also will want to use a crate to provide us with the definition of the magic constant rather than hardcoding it.

src/codegen.rs Outdated
}

let s =
read_to_string(location).map_err(|_| Diagnostic::new("GOT layout could not be read from file"))?;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Agreed!

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()),
Copy link
Member

Choose a reason for hiding this comment

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

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()),

Copy link
Author

Choose a reason for hiding this comment

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

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()));
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

Agreed!

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.
.context
.i64_type()
.const_int(0xdeadbeef00000000, false)
.const_to_pointer(llvm_type
Copy link
Member

Choose a reason for hiding this comment

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

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(())
Copy link
Member

Choose a reason for hiding this comment

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

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.
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.

2 participants