Page MenuHomePhabricator

FreeBSD kernel debugging fixes
ClosedPublic

Authored by bkoropoff on Sep 21 2017, 10:06 AM.

Details

Summary

This resolves issues I encountered when using lldb to symbolicate
FreeBSD kernel backtraces. The problems mostly centered around
FreeBSD kernel modules actually being relocatable (.o) ELF Files.

The major problems:

  • Relocations were not being applied to the DWARF debug info despite there being code to do this. Several issues prevented it from working:
    • Relocations are computed at the same time as the symbol table, but in the case of split debug files, symbol table parsing always redirects to the primary object file, meaning that relocations would never be applied in the debug file.
    • There's actually no guarantee that the symbol table has been parsed yet when trying to parse debug information.
    • When actually applying relocations, it will segfault because the object files are not mapped with MAP_PRIVATE and PROT_WRITE.
  • LLDB returned invalid results when performing ordinary address-to-symbol resolution. It turned out that the addresses specified in the section headers were all 0, so LLDB believed all the sections had overlapping "file addresses" and would sometimes return a symbol from the wrong section.

I rearranged some of the symbol table parsing code to ensure
relocations would get applied consistently and added manual calls to
make sure it happens before trying to use DWARF info, but it feels
kind of hacky. I'm open to suggestions for refactoring it.

I solved the file address problem by computing synthetic addresses for
the sections in object files so that they would not overlap in LLDB's
lookup maps.

With all these changes I'm able to successfully symbolicate backtraces
that pass through FreeBSD kernel modules. Let me know if there is a
better/cleaner way to achieve any of these fixes.

Diff Detail

Repository
rL LLVM

Event Timeline

bkoropoff created this revision.Sep 21 2017, 10:06 AM
clayborg requested changes to this revision.Sep 21 2017, 10:58 AM

Very close. See inlined comments.

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
635–636 ↗(On Diff #116212)

It would be nice to just call:

m_obj_file->PerformRelocationsIfNeeded();

If we change "void ObjectFileELF::PerformRelocations(lldb_private::Symtab *thetab)" to be "void ObjectFileELF::PerformRelocationsIfNeeded()", it can just call "m_symtab_ap.get()" to get the symbol table internally. I would rather not call ObjectFileElf::GetSymtab() as a way to perform relocations.

This revision now requires changes to proceed.Sep 21 2017, 10:58 AM
bkoropoff added inline comments.
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
635–636 ↗(On Diff #116212)

This will require changing the interface of ObjectFile, which I avoided. If that's OK, I'll add the method.

clayborg requested changes to this revision.Sep 21 2017, 1:26 PM

So I found a lot more stuff. The inlined comments detail quite a few changes we should make. The ELF relocations can use a bit of cleanup. Let me know if anything is not clear.

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
408 ↗(On Diff #116212)

It would be nice to avoid mapping everything private with copy on write if we can get away with it. Maybe we can map everything as shared and read only, and then in ObjectFile::ReadSectionData(Section *...) we relocate a private copy if needed, else hand out the const bits we have? Since most ELF files don't require modifications, only object files..

We should switch this back to false, and then re-read with "true" below if needed as detailed in comment below.

424 ↗(On Diff #116212)

We can improve this if statement to remap as private + read/write. Something like:

bool map_private = ELFHeader::NeedsRelocations(data_sp->GetBytes());

if (data_sp->GetByteSize() < length || map_private) {
  data_sp =
    DataBufferLLVM::CreateSliceFromPath(file->GetPath(), length, file_offset, map_private);

ELFHeader::NeedsRelocations would check the ELF header's e_type to see if it is llvm::ELF::ET_REL and return true. Does that make sense?

1818–1838 ↗(On Diff #116212)

Move this as a static function in lldb_private::Section:

static bool Section::IsDebugSection(SectionType sect_type);

Also rename "Type" to "sect_type". Camel case if for types in LLDB

2847 ↗(On Diff #116212)

rename "thetab" to "symtab"

2901–2905 ↗(On Diff #116212)

When would this be used? Do your .o files for the kernel binaries have two separate files? You are going to apply relocations one .o file to the symbol table in another file?

2987–2990 ↗(On Diff #116212)

So we probably want to perform relocations somewhere else, like when we parse the section headers for an ELF file. This seems fragile in that if no one accesses the symbol table for an ELF file we might end up handing out unrelocated section data. This relocation stuff should be moved. And the relocation of symbols should probably only happen when the symbol table is asked for. So we might need "m_relocated_sections" and "m_relocated_symtab". I know this is the way it was done before, but it seems error prone and we should fix this. It might be better to have lldb_private::Section objects know how to relocate themselves using its ObjectFile and the section would keep track if it has been relocated or not. ObjectFile has two ObjectFile::ReadSectionData(...) functions. Those functions could check if a Section has been relocate or not and call a new:

virtual void ObjectFile::RelocateSectionData(lldb_private::Section *section);

There are boolean bits available in lldb_private::Section (search for "m_executable" in Section.h).

source/Plugins/ObjectFile/ELF/ObjectFileELF.h
221 ↗(On Diff #116212)

This should be removed, see comments below about having lldb_private::Section objects tracking if they have been relocated or not.

299 ↗(On Diff #116212)

Ditto.

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
636 ↗(On Diff #116212)

Actually I was thinking this code was in ObjectFileELF already. I missed that it was in SymbolFileDWARF.cpp. No one outside of the ObjectFile should have to worry about applying relocations before making calls into ObjectFile subclasses. This code should be removed and put into ObjectFile subclasses. Not sure where the best place is, but no one should have to worry about manually applying relocs.

bkoropoff added inline comments.Sep 21 2017, 2:23 PM
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
408 ↗(On Diff #116212)

Maybe we can map everything as shared and read only, and then in ObjectFile::ReadSectionData(Section *...) we relocate a private copy if needed, else hand out the const bits we have?

Isn't that re-implementing copy-on-write? Is there a particular reason not to let the OS handle it, like a platform where it's unusually expensive?

1818–1838 ↗(On Diff #116212)

I realized I can just use the SHT_ALLOC bit to decide if a section needs a synthetic address, so I'll probably drop this entirely.

2901–2905 ↗(On Diff #116212)

Yes, we have split debug info for kernel binaries. Debug info files have identical section ordering and symtab, except the main sections (e.g. .text) are set to type NOBITS and aren't repeated in the file.

Come to think of it, the existing logic seems wrong for normal executables/libraries where the symtab might be stripped entirely and only be present in the debug file (our main kernel binaries retain it, so it's in both). It seems the logic should be more like:

if (m_symtab_ap != nullptr)
    return m_symtab_ap.get();

ObjectFile *module_obj_file = module_sp->GetObjectFile();
if (module_obj_file && module_obj_file != this) {
    Symtab *tab = module_obj_file->GetSymtab();
    if (tab != nullptr)
        return tab;
}

// Rest of function ...

This will find it in one file or the other, but only parse one copy as desired.

2987–2990 ↗(On Diff #116212)

Yeah, doing it just-in-time when reading the section seems cleaner.

aprantl removed a subscriber: aprantl.Sep 21 2017, 2:27 PM
clayborg added inline comments.Sep 21 2017, 2:58 PM
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
408 ↗(On Diff #116212)

As long as sending true still maps SHARED then this would be find. If it maps PRIVATE then multiple processes won't be able to share the pages... Not sure what the "true" or "false" turns into

1838 ↗(On Diff #116212)

That would be fine too

2905 ↗(On Diff #116212)

Not sure on the ramifications of your suggested change... Sounds good though, but it would change functionality. We definitely was to take the most complete symbol table...

bkoropoff added inline comments.Sep 21 2017, 3:28 PM
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
408 ↗(On Diff #116212)

It passes PRIVATE, but that's how you get copy-on-write. We don't want to share modifications.

bkoropoff added inline comments.Sep 21 2017, 4:12 PM
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
2987–2990 ↗(On Diff #116212)

ReadSectionData takes const Section *section, so if I use a boolean member on Section I'll have to make it mutable.

bkoropoff added inline comments.Sep 21 2017, 4:22 PM
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
2987–2990 ↗(On Diff #116212)

Actually, the methods themselves are const, so it's going to get ugly. Can I just make the methods and parameters non-const?

bkoropoff updated this revision to Diff 116293.Sep 21 2017, 5:34 PM
bkoropoff edited edge metadata.

Made changes according to feedback.

In particular, relocations are now applied just-in-time when reading
section data.

bkoropoff marked 15 inline comments as done.Sep 21 2017, 5:38 PM
bkoropoff added inline comments.
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
2905 ↗(On Diff #116212)

Since it's an unrelated change, I've held off for now.

clayborg requested changes to this revision.Sep 22 2017, 9:19 AM

Very nice section auto relocate code. Just a few things to clean up and this will be good to go.

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
408 ↗(On Diff #116293)

I would still like this to be false to start off with, then we determine if we have an object file, and reread the data with "true" as mentioned in comments for line 424 below. Object files are so rare. If LLDB is used for symbolication where one machine is loading multiple instances of a file, we want this shared by default.

2948 ↗(On Diff #116293)

Do symbols not need any relocation fixups anymore?

2953 ↗(On Diff #116293)

We need to check if the section has already been relocated and return right away if it has. Otherwise we will apply the relocations many times.

source/Symbol/ObjectFile.cpp
508–509 ↗(On Diff #116293)

So we either rename "RelocateSectionIfNeeded(...)" to "RelocateSection(...)", or we just call "RelocateSectionIfNeeded(section)" and let it do the check.

This revision now requires changes to proceed.Sep 22 2017, 9:19 AM
bkoropoff marked an inline comment as done.Sep 22 2017, 10:03 AM
bkoropoff added inline comments.
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
408 ↗(On Diff #116293)

PRIVATE mmaps on any sane Unix system or FILE_MAP_COPY file maps on Windows provide copy-on-write behavior. All mappings of the same file share the same physical pages until a write occurs, at which point the writer transparently receives a private copy of the page. SHARED specifies that you want to share *modifications* with other mappings and the on-disk file. If you never write to the mapping (as with executables/dylibs), copies never occur, so the end behavior is the same.

2948 ↗(On Diff #116293)

The symtab doesn't have relocations, but it's necessary to perform them. That's probably why the existing code did relocations immediately after parsing the symtab.

source/Symbol/ObjectFile.cpp
508–509 ↗(On Diff #116293)

This way avoids an unnecessary function call, so I'll rename the method.

Thanks for the explanation of the private vs shared, we can leave it as you have it. Just rename to RelocateSection and this will be good to go.

bkoropoff edited edge metadata.

Make changes according to feedback

bkoropoff marked 13 inline comments as done.Sep 22 2017, 11:03 AM
bkoropoff added inline comments.
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
2847 ↗(On Diff #116212)

That conflicts with a later local variable.

Are there any more changes needed, or is this ready to go?

clayborg accepted this revision.Sep 25 2017, 10:47 AM
This revision is now accepted and ready to land.Sep 25 2017, 10:47 AM

Since I'm unfamiliar with the workflow here, is there anything else needed from me to land these patches?

Verify you have commit priveleges, and then check in the sources with the following text in the SVN commit message:

Differential Revision: https://reviews.llvm.org/D38142

Since I'm a first-time contributor, I definitely don't have commit privileges. Should I apply for them, or is someone else supposed to commit this for me?

Since I'm a first-time contributor, I definitely don't have commit privileges. Should I apply for them, or is someone else supposed to commit this for me?

Yes, for initial contributions someone typically commits them for you. Since this is prompted by work for FreeBSD I'm happy to take care of that if @clayborg doesn' do it first.

Yes, for initial contributions someone typically commits them for you. Since this is prompted by work for FreeBSD I'm happy to take care of that if @clayborg doesn' do it first.

Feel free to do so.

I realized I don't have my LLVM SVN creds on my laptop, so will commit this when I return home in a couple of days if it's still not done.

This revision was automatically updated to reflect the committed changes.

Since I'm a first-time contributor, I definitely don't have commit privileges. Should I apply for them, or is someone else supposed to commit this for me?

Thanks for the submission! I committed this as rL314672.

If you've got more work in the pipeline you might as well apply for commit access as described in https://llvm.org/docs/DeveloperPolicy.html