-
Notifications
You must be signed in to change notification settings - Fork 33
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
process: unify load on NOMMU #611
base: master
Are you sure you want to change the base?
Conversation
ff1ed8d
to
2a6dd63
Compare
proc/process.c
Outdated
sym = (Elf32_Sym *)(symTab + (ELF32_R_SYM(rela->r_info) * sizeof(Elf32_Sym))); | ||
|
||
/* Write addend to the address */ | ||
*(char **)relptr = (char *)(sym->st_value + rela->r_addend); |
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.
This is a pretty much the only difference between SPARC and ARM stuff. Perhaps we could store strcmp result and use it here to do this operation conditionally?
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 thought about it and it was a bunny hole.
Each architecture has a separate set of Relocation Types, however relocation types are just numbers and there maybe a clash eg. R_SPARC_32
on SPARC and R_ARM_REL32
(unused now) on ARM have the same value of 3. Thus, we should compare relocation type separately on each architecture. This can be done either via ifdefs or via checking if in the ELF header the machine type(e_machine
field). Now this clash will not happen, as SPARC uses RELA and ARM uses REL. If we unify the code there is high potential for a bug in the future if more relocation types would be needed for some reason.
In my opinion the code should be modified so that:
- Getting
rela
section on an architecture usingrel
would result inENOEXEC
and vice versa - We should have separate relocations handler for each architecture
What do you think about it? Also, Should we check the architecture via ifdef or the ELF HEADER?
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.
Well, first question that has to be asked is if we'll ever need to handle any more relocation types in the kernel? Aren't dynamic linker solely in the userspace?
Regarding collisions of values of relocation types between archs - this can be solved by e.g. defining our own, abstract, machine independent relocation types on per arch basis, and handle these instead of e.g. R_SPARC_32
directly (that might be represented by e.g. HAL_R_REL32
or something along these lines).
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.
- To support static FDPIC program some 2 more relocations are needed, unless we want to move relocation process to the userspace.
Additionally if we ever want to handle C++ on NOMMU then to have proper exceptions handling there may be a need to add few more relocations. However that's a harder topic as currently exception handling causes relocations in text segment ☠️
- Nice idea!
Also my bad, both ARM and SPARC support in ABI both rela and rel, however GCC by default uses only one of those types if possible.
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.
My perspective is as follows - better for this code to do the thing it has to do nice and simple and to not prepare for the future that might never come (kinda a part of the top-down philosophy).
From my side - the proposed version is way better and status quo as is, so I have no objection to merge if you wish to not make any further changes.
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 checked and compilation with -static-pie
fixes issue of relocation in text segment in exceptions handling 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.
I have adapted code to you suggestions, and added feature to handle both Rel and Rela relocations on both targets
172480e
to
58dceed
Compare
58dceed
to
ff882dc
Compare
ff882dc
to
e448443
Compare
JIRA: RTOS-958
e448443
to
55f8566
Compare
JIRA: RTOS-958
Description
Motivation and Context
Types of changes
How Has This Been Tested?
Checklist:
Special treatment