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

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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
1172	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
1172	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
1172	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
739	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	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
94	The indentation looks a bit strange here.
sys/sys/proc.h
703	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.