Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

[stable] rt.sections_elf_shared: Upstream MIPS/RISC-V fix wrt. .so dependency names from GDC #3353

Merged
merged 1 commit into from
Feb 2, 2021

Conversation

kinke
Copy link
Contributor

@kinke kinke commented Jan 30, 2021

@kinke kinke requested a review from Burgos as a code owner January 30, 2021 13:34
@dlang-bot
Copy link
Contributor

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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

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

dub run digger -- build "stable + druntime#3353"

Comment on lines 732 to +735
version (CRuntime_Musl)
strtab = cast(const(char)*)(object.baseAddress + dyn.d_un.d_ptr); // relocate
enum relocate = true;
else version (linux)
strtab = cast(const(char)*)dyn.d_un.d_ptr;
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be CRuntime_Glibc then, instead of "linux" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not keen on looking up CRuntime_UClibc etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least the MIPS part seems to apply to uclibc too: https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/ldso/include/ldsodefs.h#n12

@ibuclaw: Do you know if there'd be a more robust way to detect this? Like checking the PF_W flag of the PT_DYNAMIC program header?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an internal detail, so no I don't think so. It did come up when the RISCV developers implemented this in gdc because it seems to have been a mistake to copy MIPS, I guess we'll just have to address that hurdle when we get to it.

@RazvanN7 RazvanN7 added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Feb 1, 2021
@Geod24 Geod24 merged commit 356b5f0 into dlang:stable Feb 2, 2021
@kinke kinke deleted the reloc branch February 2, 2021 13:32
@ibuclaw
Copy link
Member

ibuclaw commented Mar 21, 2021

Forgot that I raised a bug report about it years ago.
https://issues.dlang.org/show_bug.cgi?id=19622

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
72h no objection -> merge The PR will be merged if there are no objections raised.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants