For object files, instead of loading every sections, load the
relocatable file as how it would be laid out by the kernel linker.
Sponsored by: Juniper Networks, Inc.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 60325 Build 57209: arc lint + arc unit
Event Timeline
Comment Actions
We can probably just do this all the time without needing the flag? I think kldxref just doesn't care, or more likely ef_object.c in kldxref wasn't updated when the kernel linker changed.
Comment Actions
Ok, at least it doesn't. I just compared the result of kldxref's verbose output between the old and new version and they matches. Having said that in future there might be consumers that try to access all the sections, but we might leave it for future.
| lib/libkldelf/ef_obj.c | ||
|---|---|---|
| 316 | What's with all these cases? | |
| lib/libkldelf/ef_obj.c | ||
|---|---|---|
| 314 | In particular this is dodgy, the encoding means something else on non-amd64. Yes, amd64 is the only architecture (now that mips is gone) where a .ko is a relocatable object rather than a DSO-like thing, but we really shouldn't be assuming that. | |
| lib/libkldelf/ef_obj.c | ||
|---|---|---|
| 316 | It's rather a new introduction when KASAN's introduced, https://reviews.freebsd.org/D29049 | |
| lib/libkldelf/ef_obj.c | ||
|---|---|---|
| 316 | Firstly, it's a separate change to make that shouldn't be rolled into this (or at least if it's going to be then you should actually mention it in the commit message). Secondly, why does kldxref need to care about them? Those sections don't contain any useful data for it. It can perfectly well just leave them unloaded. | |
| lib/libkldelf/ef_obj.c | ||
|---|---|---|
| 316 | The intent of the change is actually not on whether kldxref cares or not, but rather the situation that on AMD64 since we are using relocatable object loaded sections mismatch between krtld and the userland library will produce unusable symbol lookup results because the layout's shifted. Having said that I will separate that part out to another review. | |
| lib/libkldelf/ef_obj.c | ||
|---|---|---|
| 316 | kldxref does not care about that, so long as its view is internally consistent. Relying on the addresses perfectly matching the kernel's sounds extremely fragile. If you need that, you should be getting that information out of the kernel, not attempting to replicate its logic and being thus tied to specific sets of kernels. | |
| lib/libkldelf/ef_obj.c | ||
|---|---|---|
| 316 | Which is a proposal that I previously discussed with markj@ and concluded not to proceed with, as then we have to basically expose the link_elf_obj internals as new public ABI. | |
| lib/libkldelf/ef_obj.c | ||
|---|---|---|
| 316 | You're making it ABI though now by requiring the exact same layout algorithm! Just implicitly in a fragile manner rather than explicitly. Can you please point me at this discussion? | |
| lib/libkldelf/ef_obj.c | ||
|---|---|---|
| 316 | It was on https://reviews.freebsd.org/D46537. By the way, I think we already are on the situation given that we actually tweaked that part long time ago to cater kgdb rather. | |
| lib/libkldelf/ef_obj.c | ||
|---|---|---|
| 316 | I mean, that part in link_elf_obj.c, from 58c4aee0d74d73e1a8f4a28804b40276298b54ae | |
| lib/libkldelf/ef_obj.c | ||
|---|---|---|
| 316 | Well, I've seen *that* discussion, and it certainly didn't talk about how this was a problem with the suggested approach. I still believe, and I think so does @jhb, that this is not the role of something like libkldelf, and that if you are trying to do things like that then you want a real kernel debugger like kgdb. | |
| lib/libkldelf/ef_obj.c | ||
|---|---|---|
| 316 | This patch isn't making link_elf_obj's layout algorithm part of the ABI, because kgdb already assumes it works the way it does. And the layout algorithm works the way it does in part to accommodate kgdb (and libdtrace and other hidden consumers). I don't see why it's such a problem to embed that knowledge elsewhere - yes, it's a hack, and so are .o KLDs.
kgdb isn't a substitute for a library that can reproduce the layout of a loaded KLD, not least because of licensing incompatibility. | |
| lib/libkldelf/ef_obj.c | ||
|---|---|---|
| 316 | Besides the licensing constrains, GDB isn't really design to be a library in mind. One use of this is for tools running on appliances which just need to query data from a vmcore just like how dmesg does to a vmcore file for postmortem analysis. We do not need features like stackunwinding in such case. | |