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

Some changes for readability in the cheri-architecture document #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gameboo
Copy link
Member

@gameboo gameboo commented Feb 27, 2019

These suggested changes, in my opinion, make the pseudo code easier to follow. This is the way I decided to structure things in RVBS. What do you @rmn30 think?

@gameboo gameboo requested a review from rmn30 February 27, 2019 20:30
@rmn30
Copy link
Collaborator

rmn30 commented Feb 28, 2019

Thanks for pull request. A couple of notes:

  1. A functional difference is that the operation e.g. incCapOffset is always performed even if security / sanity checks fail. This isn't really a performance concern because it would only be slower for the exceptional case but it could make a difference if, for example, I added assertions to the capability modification instructions to enforce invariants (e.g. not sealed). To me it makes sense to check all the preconditions before attempting the operation, even if you will later throw away the result in case of failure.
  2. Aesthetically, the use of new lines in if statements is inconsistent with the rest of code base as noted above. As an aside I note that these if statements were slightly over-indented to begin with. Standard is two spaces.

I'll consider changing indentation practices etc practices (e.g. using Java style braces for compactness) but needs to be done in one go as the code is currently relatively consistent.

@rmn30
Copy link
Collaborator

rmn30 commented Feb 28, 2019

Looking at the rest of cheri_insts.sail there are other places where a flatter form would make sense e.g. CSetCID and some other inconsistencies and weirdnesses...

else
writeCapReg(cd, int_to_cap(to_bits(64, getCapBase(cb_val)) + rt_val))
}
else if success then writeCapReg(cd, newCap)
Copy link
Collaborator

Choose a reason for hiding this comment

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

writeCapReg should be on new line for consistency with rest of code base. Same with all other if statements you've edited. I prefer the spaced out version.

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