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

LoongArch: prevent building duplicate "default" multilib. #70

Open
wants to merge 1 commit into
base: loongarch-12
Choose a base branch
from

Conversation

scylaac
Copy link

@scylaac scylaac commented Dec 28, 2021

No description provided.

@xry111
Copy link

xry111 commented Dec 30, 2021

I don't think it's the "correct" approach.

We should define MULTILIB_DEFAULTS for this purpose, instead of introduce those hacks. More serious problem is that the hacks may affect all targets, and we don't have resources to test all of them.

@xry111
Copy link

xry111 commented Dec 30, 2021

See #72.

@scylaac
Copy link
Author

scylaac commented Dec 30, 2021

We should define MULTILIB_DEFAULTS for this purpose, instead of introduce those hacks. More serious problem is that the hacks may affect all targets, and we don't have resources to test all of them.

I believe the problem is not so serious. Here's my reasoning:

  1. config-ml.in can be modified for a new backend.

  2. The change made to config-ml.in's architecture-independent part is not a desperate "hack" (I guess you can say that the entire multilib thing is also pretty hacky :) ). It simply provides a new choice for the ${host}s to manage their multilib layout:
    Initially, ml_toplevel_p will not be set to yes unless with_multisubdir is empty (indicating that a top-level default lib is being configured), thus it's not possible to run into the new code path unless the case ${host} clause have made an explicit assignment to with_multisubdir (which is currently only done by loongarch, and will certainly not happen for any architectures that want to code their multilib support using the "canonical" approach).
    Any ${host} type can make this assignment to indicate which multilib build is a duplicate of the "default" library (which is the one being configured). Other architecture-specific adjustments are also needed, but as you can see in this patch, it's not much nor too complex.

Although setting MULTILIB_DEFAULT is conventional, it's not really compatible with our current design:

  1. Implementing LA's architecture-specific option-handling logic with only C/C++ functions and $(tm_defines) macros (which provides better readability and flexibility to add more options). The job of MULTILIB_DEFAULT is now done by DRIVER_SELF_SPECS.

  2. Making multilib directory layout symmetric (and constant across compilers with different default ABI).

@xry111
Copy link

xry111 commented Dec 30, 2021

  1. Making multilib directory layout symmetric (and constant across compilers with different default ABI).

It will still be symmetric and constant using MULTILIB_DEFAULT. MULTILIB_DEFAULT has no effect on -print-multi-os-directory. It only affects -print-multi-lib, which is only used by gcc building process.

At now I think we should minimize the danger causing a regression in other platforms. Annoying the maintainer of an existing port is really bad in the reviewing process of a new port! :)

But if you guys can ensure that this won't break anything already existed (and convince those upstream maintainers to believe it 1), I can live with it (I don't really use multilib, even on x86_64).

Footnotes

  1. This is kind of like sending a paper: even if all of your logical sounds, the reviewer may still do not believe you! :)

@scylaac scylaac force-pushed the deduplicate-multilib branch 2 times, most recently from a79fe95 to 7a547db Compare January 4, 2022 09:50
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.

2 participants