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

fix Issue 23387 - ImportC: identical structs defined in two C files l… #14541

Merged
merged 2 commits into from
Feb 7, 2023

Conversation

WalterBright
Copy link
Member

…ead to duplicate .init symbol on macOS

Note that the test should should compile them separately:

dmd imports/maker.i -c
dmd imports/freer.i -c
dmd test23387.d -c
dmd test23387.o freer.o maker.o

But I don't see a way to do that with the test runner.

@WalterBright WalterBright added Review:Easy Review Feature:ImportC Pertaining to ImportC support labels Oct 9, 2022
compiler/test/runnable/test23387.d Outdated Show resolved Hide resolved
compiler/test/runnable/test23387.d Outdated Show resolved Hide resolved
@WalterBright WalterBright force-pushed the fix23387 branch 5 times, most recently from f97dcf5 to 02fc06c Compare October 9, 2022 07:33
@thewilsonator
Copy link
Contributor

This is a backend change, cc @ibuclaw @kinke.

compiler/test/runnable/test23387.d Outdated Show resolved Hide resolved
compiler/src/dmd/tocsym.d Outdated Show resolved Hide resolved
@ibuclaw
Copy link
Member

ibuclaw commented Oct 14, 2022

Why is dmd even generating this symbol for C sources? The initializer is all zeros, no need to store that as a run-time symbol.

@RazvanN7
Copy link
Contributor

RazvanN7 commented Nov 1, 2022

ping @WalterBright

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
23387 normal ImportC: identical structs defined in two C files lead to duplicate .init symbol on macOS

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#14541"

@WalterBright
Copy link
Member Author

I don't know what the test runner is asking for:

Test 'runnable/test23387.d' failed. The logged output:
/tmp/cirrus-ci-build/generated/freebsd/release/64/dmd -conf= -m64 -Irunnable  -fPIC  -odtest_results/runnable -c  runnable/test23387.d
/tmp/cirrus-ci-build/generated/freebsd/release/64/dmd -conf= -m64 -Irunnable  -fPIC  -odtest_results/runnable -c  runnable/imports/maker.i
/tmp/cirrus-ci-build/generated/freebsd/release/64/dmd -conf= -m64 -Irunnable  -fPIC  -odtest_results/runnable -c  runnable/imports/freer.i
/tmp/cirrus-ci-build/generated/freebsd/release/64/dmd -conf= -m64  -fPIC  -odtest_results/runnable -oftest_results/runnable/test23387_0 test_results/runnable/test23387.o test_results/runnable/maker.i test_results/runnable/freer.i
Error: cannot find input file `test_results/runnable/maker.i`
import path[0] = /tmp/cirrus-ci-build/compiler/test/../../druntime/import
import path[1] = /tmp/cirrus-ci-build/compiler/test/../../../phobos
==============================
Test 'runnable/test23387.d' failed: Expected rc == 0, but exited with rc == 1

@WalterBright
Copy link
Member Author

@dkorpel do you know what is going on with the test runner?

@adamdruppe
Copy link
Contributor

adamdruppe commented Feb 6, 2023

The C symbols shouldn't be separate anyway either. This is another case where the root fix (sticking to C semantics in importC; putting everything into one merged C namespace and collapsing compatible declarations) would solve several bugs at once, but by adding more and more special cases for individual issues, the tech debt grows instead of shrinks as issues are closed.

@WalterBright
Copy link
Member Author

@adamdruppe I understand what you mean, but do not believe it is necessary.

@dkorpel
Copy link
Contributor

dkorpel commented Feb 6, 2023

@dkorpel do you know what is going on with the test runner?

No, but I can take a closer look tomorrow

@dkorpel
Copy link
Contributor

dkorpel commented Feb 6, 2023

The test runner doesn't replace the separately compiled ".i" extension to ".o", it only replaces ".d". I added a commit that should fix it.

@dlang-bot dlang-bot merged commit 1e9affa into dlang:master Feb 7, 2023
@WalterBright WalterBright deleted the fix23387 branch February 7, 2023 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants