Page MenuHomeFreeBSD
Differential D46719

libkldelf: add a private library for kernel/kld-related ELF parsing
ClosedPublic
Actions

Authored by khng on Sep 20 2024, 4:55 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Oct 17, 6:21 AM
Unknown Object (File)
Wed, Oct 15, 10:01 PM
Unknown Object (File)
Wed, Oct 15, 10:01 PM
Unknown Object (File)
Wed, Oct 15, 11:39 AM
Unknown Object (File)
Wed, Oct 15, 11:39 AM
Unknown Object (File)
Wed, Oct 15, 11:39 AM
Unknown Object (File)
Wed, Oct 15, 11:39 AM
Unknown Object (File)
Wed, Oct 15, 12:42 AM
View All Files
Subscribers
bdrewery
emaste
imp

Details

Reviewers
jhb
andrew
jrtc27
markj
Commits
rG968bcca262a2: libkldelf: add a private library for kernel/kld-related ELF parsing
rG0a2cfd653e86: libkldelf: add a private library for kernel/kld-related ELF parsing
Summary

The libkldelf library was originally a part of kldxref(8). It exposed
ELF parsing helpers specialized in parsing KLDs and the kernel
executable. The library can be used to read metadata such as linker_set,
mod_depend, mod_version and PNP match info, and raw data from the ELF.

To promote the reuse of the facilities the ELF parsing code is separated
from kldxref(8) into a new private library.

kldxref(8) is modified to link against the libkldelf library.

Sponsored by: Juniper Networks, Inc.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 59542
Build 56429: arc lint + arc unit

Event Timeline

khng created this revision.Sep 20 2024, 4:55 PM
Herald added a reviewer: andrew. · View Herald TranscriptSep 20 2024, 4:55 PM
Herald added a reviewer: andrew. · View Herald Transcript
Herald added subscribers: emaste, imp, bdrewery. · View Herald Transcript
Harbormaster completed remote builds in B59542: Diff 143545.Sep 20 2024, 4:55 PM
khng requested review of this revision.Sep 20 2024, 4:55 PM
khng added a child revision: D46720: kldxref: use libkldxref.
khng added a reviewer: jrtc27.Sep 20 2024, 4:56 PM
jhb added a comment.Sep 21 2024, 7:57 AM

I think it would be easier to review if you moved the files out of kldxref and changed kldxref to use the library as part of this commit. Git might be able to follow history in that case, but with simple copies it is not coping it seems.

khng updated this revision to Diff 143553.Sep 21 2024, 8:27 AM

per jhb@'s suggestion.

Harbormaster completed remote builds in B59545: Diff 143553.Sep 21 2024, 8:27 AM
khng mentioned this in D46720: kldxref: use libkldxref.Sep 21 2024, 8:28 AM
khng added a comment.Sep 22 2024, 10:08 AM

I think I have to make it PRIVATELIB to be more useful.

khng retitled this revision from libkldelf: add an internal library for kernel/kld-related ELF parsing to libkldelf: add a private library for kernel/kld-related ELF parsing.Sep 23 2024, 6:07 PM
khng updated this revision to Diff 143643.Sep 23 2024, 6:08 PM

Updated to be a private library.

Harbormaster completed remote builds in B59572: Diff 143643.Sep 23 2024, 6:09 PM
khng added a reviewer: markj.Sep 23 2024, 6:09 PM
imp added a comment.Sep 23 2024, 6:12 PM

Do we need ef_mips.c still ?

khng added a comment.Sep 23 2024, 6:13 PM
In D46719#1066014, @imp wrote:

Do we need ef_mips.c still ?

At least not for the upstream FreeBSD I presume.

Ka Ho

khng added a child revision: D46764: libkldelf: add elf_lookup_symbol function.Sep 23 2024, 6:42 PM
khng updated this revision to Diff 143655.Sep 23 2024, 10:24 PM

Fix indentiation issue in mtree files

Harbormaster completed remote builds in B59575: Diff 143655.Sep 23 2024, 10:24 PM
khng updated this revision to Diff 143657.Sep 24 2024, 12:45 AM

Update Makefile.depend as well.

Harbormaster completed remote builds in B59577: Diff 143657.Sep 24 2024, 12:45 AM
markj added a comment.Sep 24 2024, 8:46 AM

This is missing some description of libkldelf's intended functionality, either in the commit message or in a man page (maybe a man page is not required for private libraries). It's hard to tell at a glance what this library does, at least for me who is not familiar with kldxref internals.

I don't have any objection to the change itself, it seems fine and will be useful for debuggers and related tools. libdtrace might be able to take advantage of it as well, replacing some existing code.

lib/libkldelf/Makefile
16

Does this mean that the header gets installed? Is that appropriate for a private library?

khng added a comment.Sep 24 2024, 2:38 PM
In D46719#1066156, @markj wrote:

This is missing some description of libkldelf's intended functionality, either in the commit message or in a man page (maybe a man page is not required for private libraries). It's hard to tell at a glance what this library does, at least for me who is not familiar with kldxref internals.

I don't have any objection to the change itself, it seems fine and will be useful for debuggers and related tools. libdtrace might be able to take advantage of it as well, replacing some existing code.

Will update the commit message accordingly.

khng updated this revision to Diff 143691.Sep 24 2024, 8:47 PM

Update commit message. If it is not reflected on Phabricator I will do it one more time.

Harbormaster completed remote builds in B59587: Diff 143691.Sep 24 2024, 8:47 PM
khng edited the summary of this revision. (Show Details)Sep 24 2024, 8:48 PM
markj added a comment.Sep 27 2024, 10:54 AM

This seems ok to me. Please make sure you test MacOS and Linux builds as well, e.g., with a PR on github.

It would be nice to see the patches for intended consumers (libkvm?) before this lands, though.

lib/libkldelf/Makefile
16

Still wondering about this.

khng updated this revision to Diff 143818.Sep 27 2024, 5:19 PM

Remove include headers installation

Harbormaster completed remote builds in B59630: Diff 143818.Sep 27 2024, 5:19 PM
khng marked an inline comment as done.Sep 27 2024, 5:20 PM
khng added inline comments.
lib/libkldelf/Makefile
16

Removed. The header is seemingly useless to be installed into usr/include for now.

markj added inline comments.Sep 30 2024, 8:26 AM
lib/libkldelf/ef_amd64.c
108

Should this case be an error instead?

khng marked an inline comment as done.Oct 2 2024, 12:58 AM
khng added inline comments.
lib/libkldelf/ef_amd64.c
108

I could be wrong, but if we return error here kmods with R_X86_64_IRELATIVE might be an issue.

markj accepted this revision.Oct 2 2024, 7:17 AM

I think @jhb should get a chance to take another look before committing this.

lib/libkldelf/ef_amd64.c
108

Indeed, but it might be good to handle that one separately somehow, e.g., return EOPNOTSUPP and modify callers to handle it.

This revision is now accepted and ready to land.Oct 2 2024, 7:17 AM
emaste added a comment.Oct 2 2024, 1:30 PM

Overall LGTM

lib/libkldelf/Makefile
7

while here, might as well make these one per line also

lib/libkldelf/ef_amd64.c
108

Agreed, silently ignoring unhandled relocations seems like a bad idea. This can be a followup change though.

khng updated this revision to Diff 144137.Oct 2 2024, 5:52 PM
khng marked an inline comment as done.

Bootstrap it everytime, and remove the __FreeBSD_version bump as well given this library is private.

This revision now requires review to proceed.Oct 2 2024, 5:52 PM
Harbormaster completed remote builds in B59690: Diff 144137.Oct 2 2024, 5:52 PM
khng edited the summary of this revision. (Show Details)Oct 2 2024, 5:52 PM
khng added a child revision: D46891: libkldelf: returns EOPNOTSUPP for unrecognizable relocations.Oct 3 2024, 8:12 PM
This revision was not accepted when it landed; it landed in state Needs Review.Oct 8 2024, 4:29 AM
Closed by commit rG0a2cfd653e86: libkldelf: add a private library for kernel/kld-related ELF parsing (authored by khng). · Explain Why
This revision was automatically updated to reflect the committed changes.
khng added a commit: rG0a2cfd653e86: libkldelf: add a private library for kernel/kld-related ELF parsing.
khng added a reverting change: rG50c64df2a173: Revert "libkldelf: add a private library for kernel/kld-related ELF parsing".Oct 8 2024, 7:49 PM
khng added a commit: rG968bcca262a2: libkldelf: add a private library for kernel/kld-related ELF parsing.Oct 18 2024, 8:22 PM

Revision Contents
Changeset List

PathSize
13 lines
lib/
libkldelf/
19 lines
653 lines
81 lines
113 lines
94 lines
97 lines
116 lines
469 lines
97 lines
85 lines
688 lines
315 lines
share/
mk/
5 lines
sys/
sys/
2 lines

Diff 143545

View Options

Makefile.inc1

Loading...
View Options

lib/libkldelf/Makefile

Loading...
View Options

lib/libkldelf/ef.c

Loading...
View Options

lib/libkldelf/ef_aarch64.c

Loading...
View Options

lib/libkldelf/ef_amd64.c

Loading...
View Options

lib/libkldelf/ef_arm.c

Loading...
View Options

lib/libkldelf/ef_i386.c

Loading...
View Options

lib/libkldelf/ef_mips.c

Loading...
View Options

lib/libkldelf/ef_obj.c

Loading...
View Options

lib/libkldelf/ef_powerpc.c

Loading...
View Options

lib/libkldelf/ef_riscv.c

Loading...
View Options

lib/libkldelf/elf.c

Loading...
View Options

lib/libkldelf/kldelf.h

Loading...
View Options

share/mk/src.libnames.mk

Loading...
View Options

sys/sys/param.h

Loading...