-
Notifications
You must be signed in to change notification settings - Fork 178
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
Refactor - Separates the text section from read-only sections #553
Conversation
7830546
to
d659b9f
Compare
63188cf
to
7fcd282
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #553 +/- ##
==========================================
+ Coverage 89.92% 90.00% +0.08%
==========================================
Files 22 22
Lines 9735 9685 -50
==========================================
- Hits 8754 8717 -37
+ Misses 981 968 -13 ☔ View full report in Codecov by Sentry. |
7fcd282
to
5cceb4f
Compare
I haven't looked at the change yet but this is not true? |
5cceb4f
to
7c70307
Compare
7c70307
to
5749444
Compare
( | ||
self.text_section_info.vaddr, | ||
&ro_section[offset..offset.saturating_add(self.text_section_info.offset_range.len())], | ||
ebpf::MM_PROGRAM_START.saturating_add(offset as u64), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing that you are adding to MM_PROGRAM_START
because you subtracted it from the offset in the initialization to maintain parallelism between this function and the get_ro_region
, right?
Would you mind explaining this in a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 237 in a400aa7
/// The first field is the offset of the section from MM_PROGRAM_START. The |
The structs field is defined as being relative to MM_PROGRAM_START
, both the text section and all read-only sections live in that MemoryRegion
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In line 469, you're subtracting the values
text_section_vaddr.saturating_sub(ebpf::MM_PROGRAM_START) as usize,
My concern was that it was a little confusing that you subtract and then you add in get_text_bytes
. A comment to explain why this is the case would be helpful.
Section::Owned(_, data) => data.capacity(), | ||
Section::Borrowed(_, _) => 0, | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we include an arm like this to account for a possible owned data (in the text section)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The text section is not supposed to ever be owned / copied. Hence why all match
expressions against it have an unreachable!
arm.
…ures. Removes SectionInfo.
5749444
to
552a629
Compare
This PR removes the indirection in which the text section is retrieved from the possibly concatenated read-only sections. There is only ever one text-section and no need to concatenate it. So instead the text section can be borrowed as a sub-slice of the relocated ELF bytes directly. Also removes the
mem_size
oftext_section_info
as that was double counted and the debug name, as it was unused.