Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SAoC] Implement -mangle-prefix #13115

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ahmetsait
Copy link

@ahmetsait ahmetsait commented Sep 30, 2021

This PR tries to implement -mangle-prefix=<path>:<prefix>

Current status:

// test.d
module test;

__gshared _MyClass_ _c_ = new _MyClass_;
__gshared _MyStruct_ _s_;

float _pi_ = 3.14f;

int _foo_(int x, _MyClass_ c, _MyStruct_ s)
{
    return x * x;
}

interface _MyInterface_
{
    long _bar_();
}

class _MyClass_ : _MyInterface_
{
    long _bar_()
    {
        return 5;
    }
}

struct _MyStruct_
{
    double d;
}

void main()
{
    (new _MyClass_)._bar_();
}
$ src/build.d && generated/linux/release/64/dmd -c -g -mangle-prefix=test.d:_prefix_ test.d && nm test.o
Success
0000000000000000 t 
                 U _D10TypeInfo_d6__initZ
0000000000000000 V _D11TypeInfo_xd6__initZ
                 U _D14TypeInfo_Class6__vtblZ
                 U _D14TypeInfo_Const6__vtblZ
                 U _D15TypeInfo_Struct6__vtblZ
                 U _D18TypeInfo_Interface6__vtblZ
0000000000000000 V _D36TypeInfo_S8_prefix_4test10_MyStruct_6__initZ
0000000000000000 V _D39TypeInfo_C8_prefix_4test13_MyInterface_6__initZ
                 U _D6Object7__ClassZ
                 U _D6object6Object5opCmpMFCQqZi
                 U _D6object6Object6toHashMFNbNeZm
                 U _D6object6Object8opEqualsMFCQtZb
                 U _D6object6Object8toStringMFZAya
0000000000000000 W _D8_prefix_4test10_MyStruct_11__xopEqualsFKxSQBrQBlQBjKxQmZb
0000000000000000 R _D8_prefix_4test10_MyStruct_6__initZ
0000000000000000 W _D8_prefix_4test10_MyStruct_9__xtoHashFNbNeKxSQBsQBmQBkZm
0000000000000000 D _D8_prefix_4test12__ModuleInfoZ
0000000000000000 V _D8_prefix_4test13_MyInterface_11__InterfaceZ
                 U _D8_prefix_4test13_MyInterface_11__InterfaceZ
0000000000000018 D _D8_prefix_4test3_c_CQtQm9_MyClass_
0000000000000020 D _D8_prefix_4test3_s_SQtQm10_MyStruct_
0000000000000000 D _D8_prefix_4test4_pi_f
0000000000000000 W _D8_prefix_4test5_foo_FiCQxQq9_MyClass_SQBmQBg10_MyStruct_Zi
0000000000000000 W _D8_prefix_4test9_MyClass_5_bar_MFZl
0000000000000000 V _D8_prefix_4test9_MyClass_6__initZ
0000000000000000 V _D8_prefix_4test9_MyClass_6__vtblZ
0000000000000000 V _D8_prefix_4test9_MyClass_7__ClassZ
0000000000000000 W _Dmain
                 U _Dmain
                 U _GLOBAL_OFFSET_TABLE_
0000000000000000 t _THUNK0
                 U __start_minfo
                 U __stop_minfo
                 U _d_dso_registry
                 U _d_newclass
                 U _d_run_main
                 U _main
0000000000000000 d internal
0000000000000000 W main

Constructive criticism is welcome.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @ahmetsait! 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 "master + dmd#13115"

src/dmd/dmangle.d Outdated Show resolved Hide resolved
src/dmd/dmangle.d Outdated Show resolved Hide resolved
src/dmd/dmangle.d Outdated Show resolved Hide resolved
@maxhaton maxhaton added the Review:student Symmetry autumn of code/Google Summer of Code label Sep 30, 2021
@ibuclaw
Copy link
Member

ibuclaw commented Oct 1, 2021

Bear in mind that I've been an outsider to the review process for saoc in general, and not knowing just how much of the implementation idea was written down in the proposal.

What rationale is there for adding yet another command-line switch to the compiler?

@maxhaton
Copy link
Member

maxhaton commented Oct 1, 2021 via email

@ibuclaw
Copy link
Member

ibuclaw commented Oct 1, 2021

This is solely intended to be available to things like dub so one can depend on multiple versions of a given module in the same dependency tree. I guess in an ideal world you'd have some other abstraction for the option since it's not really for human consumption but there isn't one (env? Probably worse) available for dmd at least.

What about prefix mapping? We already have -mv for read this module/package from this directory. But we don't have give this module/package this prefix which could be useful for reproducible builds (i.e: templates that have __PATH__ or __FULL_FILE_PATH__ encoded in their symbol).

@ahmetsait
Copy link
Author

What about prefix mapping? We already have -mv for read this module/package from this directory. But we don't have give this module/package this prefix which could be useful for reproducible builds (i.e: templates that have __PATH__ or __FULL_FILE_PATH__ encoded in their symbol).

Could you elaborate a bit on what you're trying to suggest?

src/dmd/root/dcompat.h Outdated Show resolved Hide resolved
src/dmd/globals.h Outdated Show resolved Hide resolved
src/dmd/mars.d Outdated Show resolved Hide resolved
src/dmd/mars.d Outdated Show resolved Hide resolved
src/dmd/root/filename.d Outdated Show resolved Hide resolved
src/dmd/root/filename.d Outdated Show resolved Hide resolved
src/dmd/root/dcompat.h Outdated Show resolved Hide resolved
src/dmd/mars.d Outdated Show resolved Hide resolved
@ibuclaw
Copy link
Member

ibuclaw commented Oct 8, 2021

Could you elaborate a bit on what you're trying to suggest?

When compiling modules present in a directory named old, change all references to them as if they came from directory new.
i.e:

// dmd -prefix-map=$PWD=/build pkg/mod.d
enum fullPath = __FILE_FULL_PATH__; // = "/build/pkg/mod.d"; not "/path/to/pkg/mod.d"

For example, every minor release of gdc in debian/ubuntu breaks ABI with the previous build of libphobos for the same version number, even when there's been no code changes. Why? Because the builder directory differs, and there's a number of templates that encode __FULL_FILE_PATH__ into the mangled symbol.

@ahmetsait
Copy link
Author

@ibuclaw anything else I'm missing? Also how can I reproduce that failing DAutoTest build? Not sure if it has anything to do with me.

src/dmd/globals.d Outdated Show resolved Hide resolved
src/dmd/globals.h Outdated Show resolved Hide resolved
@ahmetsait ahmetsait marked this pull request as ready for review October 10, 2021 07:45
@dkorpel
Copy link
Contributor

dkorpel commented Mar 21, 2022

Is this still being pursued?

@ahmetsait
Copy link
Author

Despite that I want to push this forward, my current focus is on my mental well-being and university studies. It is rather hard to find time and energy when every class I take requires work-heavy term projects. I would love to come back to this, but I'm afraid it is not a priority right now.

@ahmetsait
Copy link
Author

Rebased. Anything else that needs to be done or design discussions to be made?
(Not sure about the CI, seems like a random failure to me.)

@thewilsonator
Copy link
Contributor

That indeed is on old error, no idea why that is still occurring, considering you just rebased it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.