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 22567, 22568, 22571 - Remove -target option from CLI #13782

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ibuclaw
Copy link
Member

@ibuclaw ibuclaw commented Mar 8, 2022

This option is not useful for DMD given its limited range of supported targets. The option itself has been made mostly redundant by the combination of -mcpu=, -m32, -m64, and -os= flags anyway.

GDC target options are handled by the back-end, so none of this code is friendly for the "common" front-end interface.

That leaves LDC as possibly the only downstream user who may benefit from this, however there should be no reason why it can't be kept internal to LDC itself - though I'd have thought that they are already doing this with their -mtriple= option.

@dlang-bot
Copy link
Contributor

dlang-bot commented Mar 8, 2022

Thanks for your pull request, @ibuclaw!

Bugzilla references

Auto-close Bugzilla Severity Description
22567 critical Error: the architecture must not be changed in the Environment64 section of dmd.conf
22568 major -target option does nothing in compilation
22571 major [internals] -target osMajor doesn't accurately reflect actual OS version

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

@ibuclaw ibuclaw changed the title fix Issue 22567, fix Issue 22567, fix Issue 22571 - Remove -target option from CLI fix Issue 22567, 22568, 22571 - Remove -target option from CLI Mar 8, 2022
Copy link
Contributor

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

Scoring major points with this one :P

@ibuclaw
Copy link
Member Author

ibuclaw commented Mar 8, 2022

Scoring major points with this one :P

Where's my medal? :-)

@RazvanN7
Copy link
Contributor

RazvanN7 commented Mar 8, 2022

image

@thewilsonator
Copy link
Contributor

thewilsonator commented Mar 8, 2022

The option itself has been made mostly redundant by the combination of -mcpu=, -m32, -m64, and -os= flags anyway.

If you're doing cross compilation, do it right. Those mishmash of flags is not the way to do it. And that misses C/C++ runtime versions, The whole point of target is so you don't forget all those things you need to specify.

@thewilsonator
Copy link
Contributor

Note also that issue 22568 affects -os equally.

@thewilsonator
Copy link
Contributor

22567

Holy S#!& dmd argument parsing is incredulously stupid. That will take some time to figure out how to fix properly.

22571 should be pretty easy to fix properly and 22568 is moot, -os is equally affected.

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

This should be fixed properly not removed.

@ibuclaw
Copy link
Member Author

ibuclaw commented Mar 8, 2022

Note also that issue 22568 affects -os equally.

Actually, -os= works.

$ dmd hello.d -c -os=windows
$ file hello.obj 
hello.obj: Intel amd64 COFF object file, not stripped, 8 sections, symbol offset=0x154, 20 symbols
$ dmd hello.d -c -os=windows -m32
$ file hello.obj 
hello.obj: Intel 80386 COFF object file, not stripped, 6 sections, symbol offset=0x104, 14 symbols

@thewilsonator
Copy link
Contributor

I said equally, not, doesn't work. IIRC that was fixed, looks like you found that too.

@ibuclaw
Copy link
Member Author

ibuclaw commented Mar 8, 2022

This should be fixed properly not removed.

Any effort trying to improve dmd-specific driver handling is better spent elsewhere.

@ljmf00
Copy link
Member

ljmf00 commented Mar 9, 2022

This option is not useful for DMD given its limited range of supported targets. The option itself has been made mostly redundant by the combination of -mcpu=, -m32, -m64, and -os= flags anyway.

GDC target options are handled by the back-end, so none of this code is friendly for the "common" front-end interface.

That leaves LDC as possibly the only downstream user who may benefit from this, however there should be no reason why it can't be kept internal to LDC itself - though I'd have thought that they are already doing this with their -mtriple= option.

We should be careful of downstream wrappers of DMD arguments and fix them before doing this.

@ibuclaw
Copy link
Member Author

ibuclaw commented Mar 9, 2022

We should be careful of downstream wrappers of DMD arguments and fix them before doing this.

-target doesn't work, so there are doubts that there are even any downstream uses.

@ljmf00
Copy link
Member

ljmf00 commented Mar 9, 2022

We should be careful of downstream wrappers of DMD arguments and fix them before doing this.

-target doesn't work, so there are doubts that there are even any downstream uses.

On LDC, e.g., if you specify -target it won't let you use -m64 or -march to override it and it translates to -mtriple directly, but that is possible on DMD, AFAIK. If we remove -target we should ensure the same behaviour as DMD, I would say.

@ibuclaw
Copy link
Member Author

ibuclaw commented Mar 9, 2022

On LDC, e.g., if you specify -target it won't let you use -m64 or -march to override it and it translates to -mtriple directly, but that is possible on DMD, AFAIK. If we remove -target we should ensure the same behaviour as DMD, I would say.

I'd imagine that LDC would have its own option parser, and its own main function, so changes to dmd.mars won't affect it.

@ljmf00
Copy link
Member

ljmf00 commented Mar 9, 2022

On LDC, e.g., if you specify -target it won't let you use -m64 or -march to override it and it translates to -mtriple directly, but that is possible on DMD, AFAIK. If we remove -target we should ensure the same behaviour as DMD, I would say.

I'd imagine that LDC would have its own option parser, and its own main function, so changes to dmd.mars won't affect it.

I'm rather talking about ldmd and gdmd. Is, e.g. gdmd wrapping -target to something like -mtriple? In the case of LDC, it does, hence my note. See https://github.com/ldc-developers/ldc/blob/master/driver/ldmd.cpp#L587 . -os is prohibited in the ldmd wrapper.

@ibuclaw
Copy link
Member Author

ibuclaw commented Mar 9, 2022

I'm rather talking about ldmd and gdmd. Is, e.g. gdmd wrapping -target to something like -mtriple?

No.

In the case of LDC, it does, hence my note. See https://github.com/ldc-developers/ldc/blob/master/driver/ldmd.cpp#L587 . -os is prohibited in the ldmd wrapper.

Those kind of wrappers are not a downstream, unlike dub, for instance.

@ljmf00
Copy link
Member

ljmf00 commented Mar 9, 2022

I'm rather talking about ldmd and gdmd. Is, e.g. gdmd wrapping -target to something like -mtriple?

No.

Ah yeah, GCC compilers work that way, nevermind then.

In the case of LDC, it does, hence my note. See ldc-developers/ldc@master/driver/ldmd.cpp#L587 . -os is prohibited in the ldmd wrapper.

Those kind of wrappers are not a downstream, unlike dub, for instance.

I meant LDC upstream, sorry. @kinke or I can make a patch to fix this, if this is going forward.

@ibuclaw
Copy link
Member Author

ibuclaw commented Mar 9, 2022

I meant LDC upstream, sorry. @kinke or I can make a patch to fix this, if this is going forward.

Well, wrappers are free to do whatever. So nothing

I'm rather talking about ldmd and gdmd. Is, e.g. gdmd wrapping -target to something like -mtriple?

No.

Ah yeah, GCC compilers work that way, nevermind then.

Well, in practice, the only useful thing that a dmd-wrapper for gdc could do is call <target-string>-gdc, i.e: gdmd -target=mips64el-linux-gnuabi64 foo.d is transformed into mips64el-linux-gnuabi64-gdc foo.d and die if it doesn't exist. But what use would that be?

In the case of LDC, it does, hence my note. See ldc-developers/ldc@master/driver/ldmd.cpp#L587 . -os is prohibited in the ldmd wrapper.

Those kind of wrappers are not a downstream, unlike dub, for instance.

I meant LDC upstream, sorry. @kinke or I can make a patch to fix this, if this is going forward.

That the wrapper is in-tree means that it's closely tied to ldc2 anyway, so there's not much concern for it. It is free to support more options than what dmd does for "legacy" reasons.

@ljmf00
Copy link
Member

ljmf00 commented Mar 9, 2022

I meant LDC upstream, sorry. @kinke or I can make a patch to fix this, if this is going forward.

Well, wrappers are free to do whatever. So nothing

I'm rather talking about ldmd and gdmd. Is, e.g. gdmd wrapping -target to something like -mtriple?

No.

Ah yeah, GCC compilers work that way, nevermind then.

Well, in practice, the only useful thing that a dmd-wrapper for gdc could do is call <target-string>-gdc, i.e: gdmd -target=mips64el-linux-gnuabi64 foo.d is transformed into mips64el-linux-gnuabi64-gdc foo.d and die if it doesn't exist. But what use would that be?

I mean, when we want to have flags compatible for either DMD and other compilers.

I meant LDC upstream, sorry. @kinke or I can make a patch to fix this, if this is going forward.

That the wrapper is in-tree means that it's closely tied to ldc2 anyway, so there's not much concern for it. It is free to support more options than what dmd does for "legacy" reasons.

-os= is prohibited, so the flags will not be compatible.

I also tend to agree with @thewilsonator , we should try to fix it rather than remove it.

@RazvanN7
Copy link
Contributor

RazvanN7 commented Mar 10, 2022

The history of mtriple: it was merged without being implemented correctly and sat there for some time. @WalterBright came with an alternative, but the counter-argument was that mtriple should be fixed. Nobody stepped in to fix it, so Walters PR got merged. Now that path is continued by getting rid of mtriple, again the counter-argument of fixing it is made. From my perspective, the mtriple switch should have been debated and implemented correctly from the start so that such discussions wouldn't have had the chance to arise.

If there is desire to implement the mtriple thing, then the person/s that desire it should step in and propose a complete solution for it. As it stands, that is not the case. Either way, we can merge this so that the issues are fixed and @thewilsonator if you really care about this you should come up with a complete solution that is accepted by @WalterBright .

@thewilsonator
Copy link
Contributor

The history of mtriple: it was merged without being implemented correctly and sat there for some time. @WalterBright came with an alternative, but the counter-argument was that mtriple should be fixed. Nobody stepped in to fix it, so Walters PR got merged

Er, no.

-target was merged be cause there was no other flag to set the OS and it was easy to do, it was unittested, but not actually tested (because DMD does not build on macOS). Walter didn't understand how to use it, and rammed through -os ignoring the fact that 22568 affected it just as much. He then fixed that (idk which issue number), which also fixed 22568, and didn't close it. This issue is fixed.

22571 is really two issues:

  1. msvc triple that use a date don't fit into a ubyte, which is trivially fixable and not really an issue anyway see below, and
  2. for platforms other than freebsd, the number provided by the triple does not have a direct correspondence to the OS version as "seen by the user", that is e.g. for recent macOS the triple is something like darwin22, for macOS 12.1 (numbers made up. However AFAIU the only platform we care about that matters what the version number is is freebsd, which works. So, apart from some philosophical argument about the correctness of the OS major version number, this is a non-issue. We might care about it if we had something like "macosx minimum deployment target" macros that clang sets, but we don't, and until such a time as we do this remains a non-issue.

22567 is a blocker for the use of -target, one that I was unaware of until this PR. (I don't check the issue tracker frequently and I don't use DMD because I'm on Aarch64 and as far as I'm concerned DMD is dead on macOS going forward). I've not had time yet to figure out why it occurs other than verify that it does trigger it and a quick look at the code that causes the error, and I'm not sure why it is issued in the first place. I suspect the proper way to fix it would be to use a .conf like what LDC has for cross compilation, but simply deleting the error may be sufficient.

In summary: 22568 is fixed, 22571 is not a severity major issue by any stretch of the imagination, leaving 22567 which should be fixed to a state of working, and then fixed properly by having a better .conf management approach.

I will take a proper look at 22571 over the weekend.

@RazvanN7
Copy link
Contributor

So how do we get out of this stale mate? cc @WalterBright

@thewilsonator
Copy link
Contributor

Apologies I've been quite busy I'll try to get this done today or tomorrow.

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