Page MenuHomePhabricator

[lldb] [ObjectFile/ELF] Correct recognition of NetBSD images
ClosedPublic

Authored by mgorny on Feb 2 2018, 2:25 PM.

Details

Summary

[lldb] [ObjectFile/ELF] Fix recognizing NetBSD images

Split the recognition into NetBSD executables & shared libraries
and core(5) files.

Introduce new owner type: "NetBSD-CORE", as core(5) files are not tagged
in the same way as regular NetBSD executables.

Stop using incorrectly ABI_TAG and ABI_SIZE. Introduce IDENT_TAG,
IDENT_DECSZ, IDENT_NAMESZ and PROCINFO.

The new values detect correctly the NetBSD images.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

krytarowski created this revision.Feb 2 2018, 2:25 PM

Extracted from: D32149.

davide requested changes to this revision.Feb 2 2018, 2:58 PM
davide added a subscriber: davide.

Please add a test case.

This revision now requires changes to proceed.Feb 2 2018, 2:58 PM

What would the test do?

Probably take a ELF file that is NetBSD and obj2yaml it. The test would run yaml2obj on it and then test that things are recognized correctly via the SB interfaces (check triple is correct)?

You would check in the YAML code for the NetBSD ELF file so that the test case can make it into an ELF file, run the test, verify things, and then cleanup the created ELF file.

davide added a comment.Feb 2 2018, 3:28 PM

Probably take a ELF file that is NetBSD and obj2yaml it. The test would run yaml2obj on it and then test that things are recognized correctly via the SB interfaces (check triple is correct)?

The SBApi in this case is a sledgehammer. I think lldb-test or an unitest would be a better/lightweight alternative.

Is there a working example of this? I would clone an existing code for Linux or other supported OS and adapt it for NetBSD.

Please note that I'm in the process of restoration LLDB (lldb-server) so I cannot execute regular tests, at least in close time.

davide added a subscriber: zturner.Feb 2 2018, 3:33 PM

Is there a working example of this? I would clone an existing code for Linux or other supported OS and adapt it for NetBSD.

Please note that I'm in the process of restoration LLDB (lldb-server) so I cannot execute regular tests, at least in close time.

cc: @zturner / @labath

jingham added a subscriber: jingham.Feb 2 2018, 4:14 PM

There is code in the GDBRemoteTestBase that makes targets from yaml2obj. See the createTarget method in packages/Python/lldbsuite/functionalities/gdb_remote_client/gdbclientutils.py. Actually, that's method should be pulled out of the gdb_remote_client and moved to TestBase in lldbtest.py, it doesn't appear to have any dependencies on being a gdb_remote_client test, and it seems like a generally useful feature. Note that gdb_remote_client directory also has an example of making a little ELF file (a.yaml).

It looks like the GDBRemoteTestBase class also has some mechanism for cleaning up temporary files, but I'm not sure that's necessary, since the generated files should all go into the build directory now Adrian is done with that work. I don't see why the test cases should have to do this by hand.

Anyway, if that were in lldbtest (maybe createTargetFromYaml), then you could make a dotest test by copying the packages/Python/lldbsuite/test/sample_test/TestSampleTest.py somewhere, remove the build stage, and in the test body just do something like:

target = self.createTargetFromYaml(path_to_yaml)
self.assertTrue(target.GetTriple() =="whateverItsSupposedToBe", "Got the right target")

You only need to use lldb-server to run the dotest.py based tests if you need to run the process, which you don't here. You should be able to run all the tests that don't run processes, though of course if you don't have an lldb-server you can't actually launch anything regardless of what test harness is doing the launching.

labath added a comment.Feb 5 2018, 4:32 AM

Extending lldb-test's dumpModules() to also dump out the module's triple sounds like a reasonable thing to do (I am assuming that the Module class will do something reasonable when given an object file with no sections, like a core file -- if not we could make a separate command for dumping this out).

However, the tricky part here is the obj2yaml idea. Right now obj2yaml support for program headers is virtually non-existent. obj2yaml just ignores them, and yaml2obj only has basic support for PT_LOAD segments (and even here it assumes that their contents is backed by actual sections, which is not the case for core files). So, I don't think you can use yaml2obj here without some serious work to make it support this use case (which would be an awesome feature, but not exactly a trivial job...).

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
1326

Let's avoid c string functions where possible. This could be setOSName(llvm::formatv("netbsd{0}.{1}.{2}", major, minor, patch).str())

krytarowski planned changes to this revision.Feb 11 2018, 12:09 PM

I will be back to this once I will be done with debugging client-server connectivity issues (unrelated to this patch).

mgorny commandeered this revision.Feb 2 2019, 8:49 AM
mgorny added a reviewer: krytarowski.
Herald added a project: Restricted Project. · View Herald Transcript
mgorny marked an inline comment as done.Feb 18 2019, 6:20 AM
mgorny added inline comments.
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
1318

Would it be crazy to rewrite this into std::div (i.e. to get both quotient and remainder in one call)?

mgorny updated this revision to Diff 187254.Feb 18 2019, 9:06 AM
mgorny marked an inline comment as done and an inline comment as not done.
mgorny retitled this revision from Correct recognition of NetBSD images to [lldb] [ObjectFile/ELF] Correct recognition of NetBSD images.
mgorny edited the summary of this revision. (Show Details)

Added tests. I'm using yaml2obj for the regular executable, and binary file for the core dump (I've stripped it to initial 4k since that seems enough for lldb to recognize it).

mgorny marked an inline comment as done.Feb 18 2019, 9:06 AM
mgorny added inline comments.
lldb/lit/Modules/ELF/netbsd-exec.yaml
1 ↗(On Diff #187254)

I'm not sure if we should keep the whole file or try to strip this a bit.

krytarowski added inline comments.Feb 18 2019, 10:24 AM
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
1318

If it will be more readable OK.

krytarowski added inline comments.Feb 18 2019, 10:26 AM
lldb/lit/Modules/ELF/netbsd-core.test
3 ↗(On Diff #187254)

I propose to keep it as netbsd$VERSION-$ARCH.core

We will want multiple core(5) files and possible with variations (FPU layout).

mgorny updated this revision to Diff 187261.Feb 18 2019, 10:36 AM

Rename tests as requested.

krytarowski added inline comments.Feb 18 2019, 10:53 AM
lldb/lit/Modules/ELF/netbsd-core.test
3 ↗(On Diff #187254)

We will want multiple core(5) files and possible with variations (FPU layout).

Actually such variations will be applicable for https://reviews.llvm.org/D32149 Here we just want $ARCH-$VERSION.

mgorny updated this revision to Diff 187264.Feb 18 2019, 11:23 AM

Added comments on how test data was generated.

mgorny updated this revision to Diff 187350.Feb 19 2019, 5:21 AM

Reduced the test .yaml to the absolute minimum.

mgorny updated this revision to Diff 187351.Feb 19 2019, 5:34 AM

Updated to include a non-stripped core.

mgorny updated this revision to Diff 187358.Feb 19 2019, 6:25 AM

Switched to use shorter identifiers.

Looks good to me. Thank you for writing the tests. @davide, do you have anything to add here?

labath accepted this revision.Feb 19 2019, 6:39 AM
This revision was not accepted when it landed; it landed in state Needs Review.Feb 20 2019, 6:30 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2019, 6:30 AM