Page MenuHomeFreeBSD

Add load offset value to struct link_map.
ClosedPublic

Authored by kib on Tue, May 19, 2:42 PM.

Details

Summary

Do it in ABI-compatible way, we only return pointers to the structure or to the list of structures.
Except on MIPS, instread ignore ABI compat for the arch,

PR: 246561
Requested by: Damjan Jovanovic <damjan.jov@gmail.com>

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib created this revision.Tue, May 19, 2:42 PM
kib requested review of this revision.Tue, May 19, 2:42 PM
cem accepted this revision.Tue, May 19, 3:54 PM
cem added a subscriber: cem.
cem added inline comments.
sys/sys/link_elf.h
61–63 ↗(On Diff #71993)

ABI-breaking on MIPS, although I am not especially concerned about that.

This revision is now accepted and ready to land.Tue, May 19, 3:54 PM

I do wonder what the impact of changing l_addr would actually be

sys/sys/link_elf.h
61–63 ↗(On Diff #71993)

I agree that's fine for a tier-2 or lower arch.

jhb added a comment.Tue, May 19, 4:57 PM

I actually think we would be fine to fix l_addr itself. GDB doesn't use l_offs, and this will break GDB on MIPS since it knows about the layout of struct link_map already. As noted in the bug though, GDB expects l_addr to be relocbase. It only happens to work on FreeBSD currently because all of our shared libraries are linked at an address of 0 so mapbase == relocbase. If we ever wanted to do pre-linking we'd have to fix this for GDB (and presumably lldb) to find shared libraries. Right now it does mean l_addr is "wrong" for binaries, but GDB doesn't use the link_map for those. Not sure if this would affect PIE debugging on FreeBSD (i.e. is PIE debugging currently broken and would fixing l_addr fix PIE debugging? I haven't tried).

kib added a comment.Tue, May 19, 5:33 PM
In D24918#548517, @jhb wrote:

I actually think we would be fine to fix l_addr itself. GDB doesn't use l_offs, and this will break GDB on MIPS since it knows about the layout of struct link_map already. As noted in the bug though, GDB expects l_addr to be relocbase. It only happens to work on FreeBSD currently because all of our shared libraries are linked at an address of 0 so mapbase == relocbase. If we ever wanted to do pre-linking we'd have to fix this for GDB (and presumably lldb) to find shared libraries. Right now it does mean l_addr is "wrong" for binaries, but GDB doesn't use the link_map for those. Not sure if this would affect PIE debugging on FreeBSD (i.e. is PIE debugging currently broken and would fixing l_addr fix PIE debugging? I haven't tried).

I do not agree with breaking ABI, on principle. Even if gdb does not use the field that way, it is still there from 4.x times. I perhaps agree to rename the new field to l_addr, and invent some other name for current l_addr, say l_base.

kib updated this revision to Diff 72001.Tue, May 19, 5:38 PM

Reshuffle the fields.

This revision now requires review to proceed.Tue, May 19, 5:38 PM
jhb added a comment.Tue, May 19, 6:17 PM

This seems not useful, and still breaks GDB for MIPS such that I have to figure out the version of rtld (somehow) for both core dumps and live processes in the MIPS GDB backend to determine which layout of link_map to use (which I'd really rather avoid). What actual, real, software depends on l_addr being mapbase instead of relocbase? It won't be an ABI change for FreeBSD shared libraries because we link them all at 0, and it will match what other OS's do making it more likely that we will work with other portable software.

Note that while documentation for l_addr is pretty scant (no mention in psABI), what does exist does seem to suggest it being an offset: https://sourceware.org/gdb/current/onlinedocs/gdb/Library-List-Format-for-SVR4-Targets.html

lldb is a bit of a mystery code-wise, but it's "POSIXDyld" class does assume that the "base address is an offset" rather than an absolute address based on a boolean it passes as "true" when calling into the generic "DynamicLoader" code. Certainly it uses the same code for FreeBSD, NetBSD, and Linux, so it seems most likely it is also assuming relocbase.

libunwind uses dl_iterate_phdr() instead so doesn't run into this.

jhb added a comment.Tue, May 19, 6:22 PM

To be clear, I don't view changing 'l_addr' as breaking the ABI. I view it as fixing a bug where we happen to be incompatible with the rest of the world, but just got lucky since for the majority of use cases (shared libraries and rtld) the two values are identical.

kib added a comment.Wed, May 20, 1:43 PM
In D24918#548561, @jhb wrote:

To be clear, I don't view changing 'l_addr' as breaking the ABI. I view it as fixing a bug where we happen to be incompatible with the rest of the world, but just got lucky since for the majority of use cases (shared libraries and rtld) the two values are identical.

For me it does qualify as ABI breakage. And the fact that it was noticed supports this view, even if the note is that the current behavior is considered incompatible with other systems.

Well, if you prefer to have it 'fixed' instead of providing ABI-compatible extension of the struct link_map layout, let it be so. But what I do not want to keep is the l_offs for mips, both because it is not duplicates other field, and because mips is tier 2. If you still want to have gdb working with both old and new rtld, I can add an exported symbol to rtld, which presence would indicate the new layout.

kib updated this revision to Diff 72024.Wed, May 20, 1:44 PM

Change semantic of l_addr instead.

jhb added a comment.Wed, May 20, 2:00 PM

Some kind of symbol would be needed for GDB. It just means more work for me for an architecture that will probably get removed in 14 anyway. I think it would be fine to leave the field with an 'XXX: Unused comment' but remove the code that sets it from rtld if that were ok with you. That would avoid needing to patch GDB, but would remove setting it twice in rtld (since GDB doesn't actually use l_offs despite the comment's claim). I wonder if Juniper actually added l_offs as a hack due to the issue of l_addr being mapbase and that hack just got imported from Juniper but their private GDB hack to make use of it it didn't? Linux doesn't have l_offs for MIPS, so it seems to be a local invention in FreeBSD. :(

kib added a comment.Wed, May 20, 2:19 PM
In D24918#548726, @jhb wrote:

Some kind of symbol would be needed for GDB. It just means more work for me for an architecture that will probably get removed in 14 anyway. I think it would be fine to leave the field with an 'XXX: Unused comment' but remove the code that sets it from rtld if that were ok with you. That would avoid needing to patch GDB, but would remove setting it twice in rtld (since GDB doesn't actually use l_offs despite the comment's claim). I wonder if Juniper actually added l_offs as a hack due to the issue of l_addr being mapbase and that hack just got imported from Juniper but their private GDB hack to make use of it it didn't? Linux doesn't have l_offs for MIPS, so it seems to be a local invention in FreeBSD. :(

Why do you say that mips going to be removed ?

That said, why not use the tier-2 argument and change the layout. You would need to rebuild gdb to debug. IMO potential impact of changing semantic for l_addr is higher than need to rebuild gdb on mips.

I restored l_offs for now, as l_xxx. Lets make the discussion of removing it separate.

kib updated this revision to Diff 72025.Wed, May 20, 2:20 PM

Restore l_offs (as l_xxx), since it is reported as unused.

emaste accepted this revision.Wed, May 20, 2:36 PM

This is fine with me

This revision is now accepted and ready to land.Wed, May 20, 2:36 PM
jhb added a comment.Wed, May 20, 4:11 PM
In D24918#548727, @kib wrote:
In D24918#548726, @jhb wrote:

Some kind of symbol would be needed for GDB. It just means more work for me for an architecture that will probably get removed in 14 anyway. I think it would be fine to leave the field with an 'XXX: Unused comment' but remove the code that sets it from rtld if that were ok with you. That would avoid needing to patch GDB, but would remove setting it twice in rtld (since GDB doesn't actually use l_offs despite the comment's claim). I wonder if Juniper actually added l_offs as a hack due to the issue of l_addr being mapbase and that hack just got imported from Juniper but their private GDB hack to make use of it it didn't? Linux doesn't have l_offs for MIPS, so it seems to be a local invention in FreeBSD. :(

Why do you say that mips going to be removed ?

Cambridge will stop using it in another year or so due to all of their work shifting over to RISC-V. Without Cambridge supporting MIPS I suspect it will have very little active support in the developer community.

That said, why not use the tier-2 argument and change the layout. You would need to rebuild gdb to debug. IMO potential impact of changing semantic for l_addr is higher than need to rebuild gdb on mips.

Hmm, I suppose I could just change gdb's assumption, but it is a bit annoying that you won't be able to get symbols for shared libraries if you have an old core dump. Note that most of the time I've used gdb for mips it's been cross debugging, and often using a gdb built on 12.x/amd64 to debug cores from current, etc. I could change gdb so it only works for head but would no longer be able to debug cores on 12, but you can't have it do both without some kind of dynamic decision in GDB in terms of which layout of 'struct link_map' to use.

jhb accepted this revision.Wed, May 20, 4:12 PM
This revision was automatically updated to reflect the committed changes.