This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][MIPS] Core Dump Support
ClosedPublic

Authored by nitesh.jain on Feb 28 2017, 6:09 AM.

Details

Summary

This patch add support for core file. This patch includes

  • Separation of register context which was earlier share between Linux and FreeBSD.
  • Add support to analyse Linux core file for all three ABI (N32, N64 and O32)

Diff Detail

Repository
rL LLVM

Event Timeline

nitesh.jain created this revision.Feb 28 2017, 6:09 AM
labath edited edge metadata.Feb 28 2017, 8:00 AM

Have you tried running the test/testcases/functionalities/postmortem/elf-core/make-core.sh script? Does it generate a reasonably-sized core file (potentially you may need to increase the stack limit slightly). It would be great to have a mips core file test for this.

source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.h
10 ↗(On Diff #90013)

These need to stay, otherwise you'll get multiply-defined symbols on non-mips platforms.
These register contexts were meant to be used in lldb-server only and I am pretty sure you don't need them for mips core file support.

source/Plugins/Process/Utility/RegisterInfoInterface.h
32 ↗(On Diff #90013)

While I don't see anything obviously wrong about adding this interface, I am wondering why the other subclasses have not needed this.

I'd defer to @clayborg judgement on the appropriateness of the interface. What I don't like however, is that the default implementation will blatantly lie about the number of register sets for the non-mips case.

What I can suggest is to avoid putting these functions in the generic class -- you seem to be calling them from mips code only, so I don't see any immediate need to have them here. (e.g. have GetRegisterInfoInterface() cast to the appropriate type).

labath requested changes to this revision.Feb 28 2017, 8:00 AM
This revision now requires changes to proceed.Feb 28 2017, 8:00 AM
clayborg resigned from this revision.Feb 28 2017, 9:33 AM

I will let Pavel do this review as it is in his area of expertise.

eugene added a subscriber: eugene.Feb 28 2017, 11:16 AM
eugene added inline comments.
source/Plugins/Process/Utility/RegisterContextFreeBSD_mips64.cpp
31 ↗(On Diff #90013)

Here and below why not use

constexpr size_t k_num_register_sets = 1;

it seems much more straightforward than enum-for-consts trick.

104 ↗(On Diff #90013)

nullptr?

source/Plugins/Process/elf-core/ThreadElfCore.h
75 ↗(On Diff #90013)

96, 72, 128... where do these number come from?
May be they should be put them into constants and document them somehow.

nitesh.jain updated this revision to Diff 90328.Mar 2 2017, 6:29 AM
nitesh.jain edited edge metadata.

Update diff as per suggestion.

nitesh.jain marked an inline comment as done.Mar 2 2017, 6:34 AM
nitesh.jain added inline comments.
source/Plugins/Process/Utility/RegisterInfoInterface.h
32 ↗(On Diff #90013)

In case of MIPS, the register set is different for FreeBSD and Linux. In other platform, the register set may be same.

nitesh.jain marked an inline comment as done.Mar 2 2017, 6:41 AM

The .note.ABI-tag is missing in the ELF file, generated by test/testcases/functionalities/postmortem/elf-core/make-core.sh. Need to look into it.

-Thanks

labath added a comment.Mar 7 2017, 7:05 AM

Thank you for updating that. Let me know what the make_core investigation uncovers.

source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
164 ↗(On Diff #90328)

no need to fiddle with pointers:
const auto &context = static_cast<const RegisterContextLinux_mips64 &>(GetRegisterInfoInterface());

source/Plugins/Process/elf-core/ThreadElfCore.h
22 ↗(On Diff #90328)

Thank you for introducing these constants. Since we already started using constexpr variables for constants, let's make these such as well. You could even hide these into the cpp file if you move the GetSize function there (it's grown so big it does not belong to a header anyway).

Update Diff as per suggestion

Thank you for updating that. Let me know what the make_core investigation uncovers.

In case of MIPS, the core file doesn't contain Arch and OS information. Hence we shared information from executable. The make_core use "-nostdlib" flag because of which .note.ABI-tag is missing in the ELF file. Hence OS is set to unknown. I will look into it and submit a separate patch for that.

Thanks

In case of MIPS, The ELFLinuxPrPsInfo.pr_uid and ELFLinuxPrPsInfo.pr_gid is always 32 bit irrespective of platforms

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

Thank you.

This revision is now accepted and ready to land.Mar 23 2017, 9:26 AM
This revision was automatically updated to reflect the committed changes.