Page MenuHomePhabricator

[LLDB][MIPS] Fix Core file Architecture and OS information
ClosedPublic

Authored by nitesh.jain on Mar 23 2017, 12:48 AM.

Details

Summary

The MIPS core file doesn't contain any Architecture revision, OS and ABI information. Hence we need to relied on executable to get that information. This patch also add core file test cases for MIPS.

Patch By:- Bhushan

Diff Detail

Repository
rL LLVM

Event Timeline

nitesh.jain created this revision.Mar 23 2017, 12:48 AM
labath added inline comments.Mar 23 2017, 3:52 AM
source/Plugins/Process/elf-core/ProcessElfCore.cpp
675 ↗(On Diff #92769)

I'm not terribly happy with the default-to-host mode here, particularly as we already have some code which detects linux in ObjectFileELF::RefineModuleDetailsFromNote. I'm not terribly happy about that either, but I hope we could at least have just one dodgy detection code.

Did you check whether it's possible to extend that function to cover mips as well (probably the NT_FILE part, which searches for some i386-linux-gnu paths in the binary) ?

labath edited edge metadata.Mar 23 2017, 3:54 AM

BTW, thank you for adding the test. It's not obvious from the patch: how big are the core files?

They are around ~28KB

nitesh.jain added inline comments.Mar 23 2017, 4:15 AM
source/Plugins/Process/elf-core/ProcessElfCore.cpp
675 ↗(On Diff #92769)

In our case , files path doesn't contain any Linux string.

nin@debian-co3-1:~/LLVM-new/llvm/tools/lldb/packages/Python/lldbsuite/test/functionalities/postmortem/elf-core$ readelf -a linux-mips64el-gnuabi64.core
ELF Header:

Magic:   7f 45 4c 46 02 01 01 00 00 00 00 00 00 00 00 00
Class:                             ELF64
Data:                              2's complement, little endian
Version:                           1 (current)
OS/ABI:                            UNIX - System V
ABI Version:                       0
Type:                              CORE (Core file)
Machine:                           MIPS R3000
Version:                           0x1
Entry point address:               0x0
Start of program headers:          64 (bytes into file)
Start of section headers:          0 (bytes into file)
Flags:                             0x0
Size of this header:               64 (bytes)
Size of program headers:           56 (bytes)
Number of program headers:         6
Size of section headers:           0 (bytes)
Number of section headers:         0
Section header string table index: 0

There are no sections in this file.

There are no sections to group in this file.

Program Headers:

Type           Offset             VirtAddr           PhysAddr
               FileSiz            MemSiz              Flags  Align
NOTE           0x0000000000000190 0x0000000000000000 0x0000000000000000
               0x0000000000000644 0x0000000000000000         0x0
LOAD           0x0000000000001000 0x0000000120000000 0x0000000000000000
               0x0000000000001000 0x0000000000001000  R E    0x1000
LOAD           0x0000000000002000 0x000000fff2a60000 0x0000000000000000
               0x0000000000001000 0x0000000000001000  R      0x1000
LOAD           0x0000000000003000 0x000000fff2a61000 0x0000000000000000
               0x0000000000001000 0x0000000000001000  R E    0x1000
LOAD           0x0000000000004000 0x000000ffffc01000 0x0000000000000000
               0x0000000000002000 0x0000000000002000  RWE    0x1000
LOAD           0x0000000000006000 0x000000fffffff000 0x0000000000000000
               0x0000000000000000 0x0000000000001000  RWE    0x1000

There is no dynamic section in this file.

There are no relocations in this file.

The decoding of unwind sections for machine type MIPS R3000 is not currently supported.

Dynamic symbol information is not available for displaying symbols.

No version information found in this file.

Displaying notes found at file offset 0x00000190 with length 0x00000644:

Owner                 Data size       Description
CORE                 0x000001e0       NT_PRSTATUS (prstatus structure)
CORE                 0x00000088       NT_PRPSINFO (prpsinfo structure)
CORE                 0x00000080       NT_SIGINFO (siginfo_t data)
CORE                 0x00000130       NT_AUXV (auxiliary vector)
CORE                 0x000000aa       NT_FILE (mapped files)
  Page size: 4096
               Start                 End         Page Offset
  0x0000000120000000  0x0000000120001000  0x0000000000000000
      /home/nin/LLVM-new/llvm/tools/lldb/packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/linux-mips64el-gnuabi64.out
labath added inline comments.Mar 23 2017, 4:28 AM
source/Plugins/Process/elf-core/ProcessElfCore.cpp
675 ↗(On Diff #92769)

That's a good point. In fact that is the case on the x86 core files as well we have checked in as well, as their executables were linked statically. Only, I guess in the x86 case we don't rely on the os that much.

Will this affect the ability of the tests to run reproducibly (on windows for example), or will the os be picked up from the executable if it is present? If it won't then we'll need to think hard about how to make the tests deterministic.

nitesh.jain added inline comments.Mar 23 2017, 7:00 AM
source/Plugins/Process/elf-core/ProcessElfCore.cpp
675 ↗(On Diff #92769)

If Host OS is windows then this test case will fail. The MIPS support freeBSD, Linux and Android. The note.n_name == LLDB_NT_OWNER_GNU is valid for Linux platform. Hence we can used this information to set OS.

labath added inline comments.Mar 23 2017, 7:25 AM
source/Plugins/Process/elf-core/ProcessElfCore.cpp
675 ↗(On Diff #92769)

That sounds better. Can we try that?

updated diff as per suggestion

Thank you for that.

I just have one more concern. I see two places where you are copying the archspec from core file to the target and then back again. I must be missing something, because that looks completely unnecessary at first sight. What is the reason behind that? Why can't we leave ProcessElfCore::GetArchitecture as is ?

source/Plugins/Process/elf-core/ProcessElfCore.cpp
220 ↗(On Diff #92808)

Here you copy the architecture from core to target.

668 ↗(On Diff #92808)

And here you fetch the target instead of the core architecture.

nitesh.jain added inline comments.Mar 23 2017, 9:02 AM
source/Plugins/Process/elf-core/ProcessElfCore.cpp
220 ↗(On Diff #92808)

Actually we are skipping it , if the arch is MIPS ( ! arch.IsMIPS()).

The core architecture doesn't have any revision, os and ABI information. Hence we need to relied on target architecture.

labath accepted this revision.Mar 23 2017, 9:25 AM

Ah, that explains it, thanks.

Let's give Greg a chance to comment, but otherwise I am fine with this.

This revision is now accepted and ready to land.Mar 23 2017, 9:25 AM
clayborg edited edge metadata.Mar 23 2017, 10:55 AM

Let me know what you think about the MergeFrom comment. I am generally ok with this, but just wanted to check in case the merge made any sense in this patch somewhere.

source/Plugins/Process/elf-core/ProcessElfCore.cpp
220 ↗(On Diff #92808)

So the target has an incomplete architecture here? If that is true, you should probably get the architecture and merge it. You can merge any missing info with:

ArchSpec target_arch = GetTarget().GetArchitecture();
ArchSpec core_arch(m_core_module_sp->GetArchitecture());
target_arch.MergeFrom(core_arch);
GetTarget().SetArchitecture(target_arch);

Not sure how that would work here, but we should be using this. If we have a target whose arch is "x86_64-apple-macosx" and you have a core file that is "mips-..." an error should be produced.

Update diff as per suggestion.

Thanks

clayborg accepted this revision.Mar 27 2017, 7:54 AM

Looks good

This revision was automatically updated to reflect the committed changes.