Details

Reviewers

kib
markj

Commits

rGd99bbd1f1601: Add a link to the Elf_Brandinfo into the struc proc.
rG615f22b2fb6d: Add a link to the Elf_Brandinfo into the struc proc.

Summary

To allow the ABI to make a dicision based on the Brandinfo add a link
to the Elf_Brandinfo into the struct sysentvec. Add a note that the high 8 bits
of Elf_Brandinfo flags is private to the ABI.

This can be done by adding a private_flag filed to the struct sysentvec,
but it requires a lot of more work - duplicate of sysentvec, add some fileds
to the struct sysentvec which is private to the ABI and so on.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 40129
Build 37018: arc lint + arc unit

Event Timeline

dchagin created this revision.Jun 27 2021, 3:30 PM

Herald added a subscriber: imp. · View Herald TranscriptJun 27 2021, 3:30 PM

dchagin requested review of this revision.Jun 27 2021, 3:30 PM

Harbormaster completed remote builds in B40129: Diff 91412.Jun 27 2021, 3:30 PM

dchagin edited the summary of this revision. (Show Details)Jun 27 2021, 3:32 PM

dchagin added reviewers: kib, markj.

dchagin changed the repository for this revision from rS FreeBSD src repository - subversion to rG FreeBSD src repository.

kib added inline comments.Jun 27 2021, 3:41 PM

sys/kern/imgact_elf.c
1174	How could this work? Imagine you have two brands pointing to the same sysent. Then the latest executed brand would always override brand_info in sysvec.

dchagin added inline comments.Jun 27 2021, 3:53 PM

sys/kern/imgact_elf.c
1174	How could this work? Imagine you have two brands pointing to the same sysent. Then the latest executed brand would always override brand_info in sysvec. whoops, indeed. thanks. ugh, i need a knowledge about which is brand is in use, but the second way that i see is not good.

kib added inline comments.Jun 27 2021, 4:09 PM

sys/kern/imgact_elf.c
1174	Store the brand in struct proc?

done, nice, thank you)))

Harbormaster completed remote builds in B40130: Diff 91413.Jun 27 2021, 4:29 PM

kib added inline comments.Jun 27 2021, 5:05 PM

sys/sys/proc.h
737 ↗	(On Diff #91413)	This should be named like p_elf_brand_info. Also, I believe it should be in the section that is copied on fork. Also, add a proper locking annotation for the new field (the (X) note in other comments).

done

Harbormaster completed remote builds in B40131: Diff 91416.Jun 27 2021, 5:37 PM

done

Harbormaster completed remote builds in B40132: Diff 91417.Jun 27 2021, 5:41 PM

asserts fixed, tinderboxed

Harbormaster completed remote builds in B40133: Diff 91418.Jun 27 2021, 5:58 PM

kib added inline comments.Jun 27 2021, 7:38 PM

sys/sys/proc.h
706 ↗	(On Diff #91418)	You do not clear it on exec, so the note about NULL for non-elf binaries is not true. I am not sure if this needs fixing. Also note the spelling, it is ELF, not Elf.

s/Elf/ELF/, I think p_elf_brandinfo should be saved and cleared before img activator
work, and restored on error like p_osrel.

Harbormaster completed remote builds in B40135: Diff 91423.Jun 27 2021, 10:47 PM

kib accepted this revision.Jun 28 2021, 11:03 AM

This revision is now accepted and ready to land.Jun 28 2021, 11:03 AM

markj accepted this revision.Jun 28 2021, 1:12 PM

markj added inline comments.

sys/sys/imgact_elf.h
92	The indentation looks a bit strange here.
sys/sys/proc.h
703 ↗	(On Diff #91423)	I think `p_elf_machine` is redundant now, isn't it always equal to `p_elf_brandinfo->machine`? Maybe not worth addressing in this review.