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

Changes to better handle relocation #33

Merged
merged 1 commit into from
Jul 19, 2024
Merged

Conversation

0152la
Copy link
Contributor

@0152la 0152la commented Jul 18, 2024

  • Record relocation shndx, so we know this is either an external, or a local symbol (improves lookup logic)
  • Add function to do symbol look-up per library, than per compartment
  • Fix amount of memory available in comp_utils when not going through a compartment
  • Add some more tests (currently not working: toupper and lua_suite_some)

This should've fixed at least toupper, but (hopefully) it's at least a step forward.

@0152la 0152la requested a review from ltratt July 18, 2024 15:25
src/compartment.c Outdated Show resolved Hide resolved
src/compartment.c Outdated Show resolved Hide resolved
cond = cond && curr_sym.sym_offset != 0;
}

if (cond)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be clearer to say if (cond && curr_sym.sym_type == sym_type && curr_sym.sym_offset != 0) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer it this way, since then I have each condition separate, with a clarifying comment. I would assume the compiler will collapse it down to something better.

Copy link
Contributor

Choose a reason for hiding this comment

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

I found this really hard to read, because it's so unconventional, and it took me a while to convince myself it was intentional. Another way around this is to use a nested if with a comment at each level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per our offline talk, I've added some additional clarifying comments about the use of cond here - faf4545.

@ltratt
Copy link
Contributor

ltratt commented Jul 19, 2024

Please squash.

* Record relocation `shndx`, so we know this is either an external,
  or a local symbol (improves lookup logic)
* Add function to do symbol look-up per library, than per compartment
* Fix amount of memory available in `comp_utils` when not going through
  a compartment
* Add some more tests (currently not working: `toupper` and
  `lua_suite_some`)
@0152la
Copy link
Contributor Author

0152la commented Jul 19, 2024

Squashed.

@ltratt ltratt added this pull request to the merge queue Jul 19, 2024
Merged via the queue into capablevms:master with commit 12e7ab2 Jul 19, 2024
2 checks passed
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