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

Be less conservative wrt. linker dead code elimination #4320

Merged
merged 4 commits into from
Feb 22, 2023

Conversation

kinke
Copy link
Member

@kinke kinke commented Feb 10, 2023

No description provided.

@kinke kinke force-pushed the dead_strip branch 2 times, most recently from 74132fa to 5c76e42 Compare February 11, 2023 12:42
@kinke kinke marked this pull request as ready for review February 11, 2023 12:43
@kinke
Copy link
Member Author

kinke commented Feb 11, 2023

Ouch, Apple's ld64 apparently strips rt_options globals from executables linked against shared druntime. This might be a blocker for Apple targets.

…_enabled} druntime symbol overrides

Required to fix lit-test `linking/rt_options.d` on Apple targets when
linking against *shared* druntime.
@kinke
Copy link
Member Author

kinke commented Feb 11, 2023

Very unsure about the last [tentative fix] commit - has this really just worked accidentally so far (and only started to show up on macOS with shared druntime now with dead_strip)? It's about

// Issue 19593
assert(!("aaa" >= "bbb"));
assert("aaa" <= "bbb");

@kinke
Copy link
Member Author

kinke commented Feb 12, 2023

The size of the bundled D executables (except for ldc2 and ldc-profdata, which are unchanged) from the macOS-x64 CI package is cut in half - from 59 MB to 30.4 MB; e.g., ldmd2 from 4.4 to 1.2 MB.

@JohanEngelen
Copy link
Member

JohanEngelen commented Feb 12, 2023

Very unsure about the last [tentative fix] commit - has this really just worked accidentally so far (and only started to show up on macOS with shared druntime now with dead_strip)? It's about

// Issue 19593
assert(!("aaa" >= "bbb"));
assert("aaa" <= "bbb");

Sorry I don't fully understand. Why was the fix needed? (what does adding check=assert=on change for this test?)

@kinke
Copy link
Member Author

kinke commented Feb 12, 2023

The produced .def file didn't include the two string-compare functions (dstrcmp and __cmp) expected in https://github.com/ldc-developers/ldc/blob/95ac5cadb89e2fde1d9501f3aac2dc8a077c5278/runtime/druntime/test/profile/mytrace.def.exp - on macOS only, and only when linking shared druntime (tested by 2 CI jobs, both failed), and only with dead_strip. I don't know anything about how this profiling code works; and how the 2 functions were apparently in there before the dead_strip, even though the string comparisons are AFAICT exclusively triggered behind assertions, which are ignored with -release when running that test in release mode.

@JohanEngelen
Copy link
Member

The produced .def file didn't include the two string-compare functions (dstrcmp and __cmp) expected in https://github.com/ldc-developers/ldc/blob/95ac5cadb89e2fde1d9501f3aac2dc8a077c5278/runtime/druntime/test/profile/mytrace.def.exp - on macOS only, and only when linking shared druntime (tested by 2 CI jobs, both failed), and only with dead_strip. I don't know anything about how this profiling code works; and how the 2 functions were apparently in there before the dead_strip, even though the string comparisons are AFAICT exclusively triggered behind assertions, which are ignored with -release when running that test in release mode.

yeah ok... I'm OK with your fix :)

@kinke kinke merged commit aa9b278 into ldc-developers:master Feb 22, 2023
@kinke kinke deleted the dead_strip branch February 22, 2023 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants