Refactor ELF utils from rt.backtrace.elf and rt.sections_elf_shared to core.internal.elf.{dl,io} #2330

kinke · 2018-10-14T21:53:09Z

A common place for working with ELF binaries, accessible by user code as well and providing a bit more convenience.

dlang-bot · 2018-10-14T21:53:09Z

Thanks for your pull request and interest in making D better, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	19322	regression	A lot of memory is consumed and not freed to the system when Exception is formatted with stacktrace in debug

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

To target stable perform these two steps:

Rebase your branch to upstream/stable:

git rebase --onto upstream/stable upstream/master

Change the base branch of your PR to stable

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2330"

jacob-carlborg

Missing Ddoc documentation for all new public symbols.

src/core/elf.d

jacob-carlborg · 2018-10-15T08:11:13Z

src/core/elf.d

+        }
+    }
+
+    dl_phdr_info info;


Do we want this to be directly exposed?

I think so; rt.sections_elf_backtrace currently needs it for the TLS stuff, and I don't want to abstract it fully away, particularly since the struct is OS-dependent.

src/rt/backtrace/elf.d

dnadlinger · 2018-10-15T10:18:06Z

Do we really want this in core (i.e. user-exposed)? Even after the refactoring, the code consists mostly of cobbled-together pieces that fit our need rather than a universal API.

kinke · 2018-10-15T10:32:30Z

That was my main goal, letting the user have access to this functionality too. E.g., we could presumably use it in LDC to parse the dependencies of an .so to be linked; if it depends on shared druntime/Phobos, LDC would use -link-defaultlib-shared to link the binary against the shared default libs too, preventing the runtime error 'recompile with -link-defaultlib-shared' for executables compiled without it and linked with shared D libs.

dnadlinger · 2018-10-15T10:52:15Z

That was my main goal, letting the user have access to this functionality too.

But why druntime over a separate library? Adding stuff into core.* tends to accrete quite a bit of scrutiny and discussion, and rightly so.

kinke · 2018-10-15T13:12:20Z

Well, because we need the stuff in druntime too. A higher level lib can always build on it.

iain-buclaw-sociomantic · 2018-10-16T14:40:48Z

I don't think it's wise to have a platform-specific module in a common place.

kinke · 2018-10-16T14:56:47Z

I'm totally flexible wrt. final place of this; it could live in core.sys.elf or so too, it's just that I haven't found any suited existing dir yet (afaik not suited for Posix due to Apple's MachO).

joakim-noah · 2018-10-23T02:02:45Z

Needs rebase and fix merge.

joakim-noah · 2018-10-25T06:42:13Z

src/core/elf.d

+@nogc nothrow:
+    this(ref const ElfFile file, size_t index)
+    {
+        assert(Elf_Shdr.sizeof == file.ehdr.e_shentsize);


Tried my upcoming Android commit for #2325 with this pull, but it segfaults here: will look into why. In the meantime, turn this into an abort message, just as you did with #2324, so it exits immediately rather than repeatedly trying to throw an exception and then segfaulting. I'm doing the same in my Android pull, replacing asserts with abort messages.

This is probably because this module currently only supports native ELF files, i.e., corresponding with the host, which is quite a serious limitation.

I don't think aborting here is a good idea; #2324 was different, as the chances for the GC not being initialized yet/already shut down are very high for that code, so throwing in that code is to be avoided.

I'm building on Android itself, but it's likely that some of the ELF definitions are different there.

As for aborting here, I will be using this on Android before the GC is initialized, but I didn't check how you're using it.

It doesn't matter how I'm using it; if this code is in core, the user is supposed to be able to use it too, so I don't want to abort in case the ELF file doesn't match; it might as well just be an attempt of opening the file as e.g. 32-bit ELF file.
So I should actually change the assertion to a normal throw (oh well, not sure how well that plays with nothrow and the backtrace code). Edit: Ah, that's the ElfSectionHeader ctor; using that one should be done after checking ElfFile.isValid(), so the assertion is fine IMO.

joakim-noah · 2018-12-06T20:06:27Z

@kinke, any chance we can get this into the final 1.13 release? I'd like to use it for Android too.

kinke · 2018-12-06T20:09:08Z

I don't think so; I'm not too keen on having this pretty big diff wrt. upstream in LDC.

joakim-noah · 2018-12-06T20:13:03Z

No, I mean do you think you can get it merged here in time for a cherrypick before the 1.13 release?

kinke · 2018-12-06T20:13:38Z

Lol, no way! :)

joakim-noah · 2018-12-06T20:18:44Z

Heh, OK, I still need to look into why this pull doesn't work on Android anyway, maybe I'll just try to get it working for the native Android builds of LDC.

Incl. some minor refactoring (pointer to ref, make `findSectionByName()` an ElfFile method, add `findSectionHeaderByName()` convenience method etc.).

…efore use

…bjects

I.e., allowing to read non-native ELF files too. The byte order must match the target's though.

kinke · 2020-01-17T19:44:34Z

Rebased and moved to core.internal.elf.{dl,io}.

PetarKirov · 2020-01-22T08:44:58Z

Let's stick to core.internal.elf.{dl,io} for now. We could always change the color of the bikeshed if/when we promote the module to a non-internal core package.

kinke · 2020-02-21T13:31:48Z

Ping ding dong, another month has passed by.

issues resolved.

CyberShadow · 2021-02-22T11:01:15Z

@kinke This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=21656

kinke requested review from andralex, Burgos and wilzbach as code owners October 14, 2018 21:53

kinke mentioned this pull request Oct 14, 2018

druntime: Test core.elf refactoring ldc-developers/ldc#2875

Closed

kinke force-pushed the elf_upstream branch from e1b2be4 to 19734f0 Compare October 14, 2018 22:11

jacob-carlborg previously requested changes Oct 15, 2018

View reviewed changes

src/core/elf.d Outdated Show resolved Hide resolved

src/core/elf.d Outdated Show resolved Hide resolved

src/core/elf.d Outdated Show resolved Hide resolved

jacob-carlborg reviewed Oct 15, 2018

View reviewed changes

src/rt/backtrace/elf.d Outdated Show resolved Hide resolved

joakim-noah reviewed Oct 25, 2018

View reviewed changes

joakim-noah mentioned this pull request Nov 9, 2018

Bionic: Add x64 declarations, switch to single TLS function, and define __USE_GNU #2325

Merged

thewilsonator added the Needs Rebase needs a `git rebase` performed label Nov 9, 2018

kinke force-pushed the elf_upstream branch from 19734f0 to 3baaece Compare December 8, 2018 02:41

dlang-bot removed the Needs Rebase needs a `git rebase` performed label Dec 8, 2018

kinke force-pushed the elf_upstream branch 2 times, most recently from 07c90db to e716ef7 Compare December 8, 2018 14:39

dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Dec 8, 2018

kinke force-pushed the elf_upstream branch from 6c2407a to 058c1e5 Compare December 8, 2018 16:48

kinke added 16 commits January 17, 2020 20:40

Extract ELF section reading from rt.backtrace.elf to core.elf

fab4a68

Incl. some minor refactoring (pointer to ref, make `findSectionByName()` an ElfFile method, add `findSectionHeaderByName()` convenience method etc.).

rt.backtrace.elf: Prevent munmap()ping the .debug_line section data b…

9179728

…efore use

core.elf: Support simple iteration over the currently loaded shared o…

2e4b225

…bjects

core.elf: Avoid code duplication

296dc2f

core.elf: Add ElfFile convenience ctor taking a file descriptor

65712a9

Move some functionality from rt.sections_elf_shared to core.elf

fe6639d

Slight core.elf refactoring

d006561

Slightly simplify rt.backtrace.elf

0c3f5aa

core.elf: Refactor file-based memory-mapped I/O a bit

13dbc3f

core.elf: Prepare for generic ELF I/O

3f1392d

I.e., allowing to read non-native ELF files too. The byte order must match the target's though.

Split core.elf into core.elf.{dl,io}

3c77e41

Fix Issue 19322 (memory leak in backtrace generation with -g)

0fa014f

Extend documentation for core.elf.io

abc352d

Minor documentation extensions

332cf0a

Fixup Makefiles

40370b8

Move core.elf.* to core.internal.elf.*

cb4d513

kinke force-pushed the elf_upstream branch from b9b46a9 to cb4d513 Compare January 17, 2020 19:43

thewilsonator approved these changes Feb 21, 2020

View reviewed changes

thewilsonator added the auto-merge label Feb 21, 2020

dlang-bot merged commit f80a07e into dlang:master Feb 21, 2020

kinke deleted the elf_upstream branch February 21, 2020 15:07

kinke mentioned this pull request Mar 8, 2020

GC on Android SIGSEGV ldc-developers/ldc#3350

Closed

kinke restored the elf_upstream branch March 12, 2020 19:44

kinke mentioned this pull request Mar 12, 2020

core.internal.elf: Improve usability #2983

Merged

kinke deleted the elf_upstream branch March 12, 2020 20:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ELF utils from rt.backtrace.elf and rt.sections_elf_shared to core.internal.elf.{dl,io} #2330

Refactor ELF utils from rt.backtrace.elf and rt.sections_elf_shared to core.internal.elf.{dl,io} #2330

kinke commented Oct 14, 2018

dlang-bot commented Oct 14, 2018 •

edited

jacob-carlborg left a comment

jacob-carlborg Oct 15, 2018

kinke Oct 15, 2018

dnadlinger commented Oct 15, 2018

kinke commented Oct 15, 2018

dnadlinger commented Oct 15, 2018

kinke commented Oct 15, 2018 •

edited

iain-buclaw-sociomantic commented Oct 16, 2018

kinke commented Oct 16, 2018

joakim-noah commented Oct 23, 2018

joakim-noah Oct 25, 2018

kinke Dec 6, 2018

joakim-noah Dec 15, 2018

kinke Dec 15, 2018 •

edited

joakim-noah commented Dec 6, 2018

kinke commented Dec 6, 2018

joakim-noah commented Dec 6, 2018

kinke commented Dec 6, 2018

joakim-noah commented Dec 6, 2018

kinke commented Jan 17, 2020

PetarKirov commented Jan 22, 2020 •

edited

kinke commented Feb 21, 2020

CyberShadow commented Feb 22, 2021

Refactor ELF utils from rt.backtrace.elf and rt.sections_elf_shared to core.internal.elf.{dl,io} #2330

Refactor ELF utils from rt.backtrace.elf and rt.sections_elf_shared to core.internal.elf.{dl,io} #2330

Conversation

kinke commented Oct 14, 2018

dlang-bot commented Oct 14, 2018 • edited

Bugzilla references

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

Testing this PR locally

jacob-carlborg left a comment

Choose a reason for hiding this comment

jacob-carlborg Oct 15, 2018

Choose a reason for hiding this comment

kinke Oct 15, 2018

Choose a reason for hiding this comment

dnadlinger commented Oct 15, 2018

kinke commented Oct 15, 2018

dnadlinger commented Oct 15, 2018

kinke commented Oct 15, 2018 • edited

iain-buclaw-sociomantic commented Oct 16, 2018

kinke commented Oct 16, 2018

joakim-noah commented Oct 23, 2018

joakim-noah Oct 25, 2018

Choose a reason for hiding this comment

kinke Dec 6, 2018

Choose a reason for hiding this comment

joakim-noah Dec 15, 2018

Choose a reason for hiding this comment

kinke Dec 15, 2018 • edited

Choose a reason for hiding this comment

joakim-noah commented Dec 6, 2018

kinke commented Dec 6, 2018

joakim-noah commented Dec 6, 2018

kinke commented Dec 6, 2018

joakim-noah commented Dec 6, 2018

kinke commented Jan 17, 2020

PetarKirov commented Jan 22, 2020 • edited

kinke commented Feb 21, 2020

CyberShadow commented Feb 22, 2021

dlang-bot commented Oct 14, 2018 •

edited

kinke commented Oct 15, 2018 •

edited

kinke Dec 15, 2018 •

edited

PetarKirov commented Jan 22, 2020 •

edited