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 13292 - DMD accepts both -m32 and -m64 #11058

Closed
wants to merge 1 commit into from

Conversation

wolframw
Copy link
Contributor

Now raises an error when conflicting -mXXX arguments are given.
However, multiple -mXXX are still valid, iff all XXX are the same.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @wolframw! 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

Auto-close Bugzilla Severity Description
13292 normal DMD accepts both -m32 and -m64

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

@wolframw wolframw force-pushed the fix_13292 branch 3 times, most recently from 4dc13ab to f7d0d51 Compare April 25, 2020 16:15
@wolframw
Copy link
Contributor Author

I did not expect destroying other tests with this change... I'll have a look on it later.

Copy link
Contributor

@MoonlightSentinel MoonlightSentinel left a comment

Choose a reason for hiding this comment

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

I don't think we should do this because it's pretty common to allow flags to override previous flags. E.g. gcc allows both -m32 -m64 and other dmd flags can be specified mutliple times as well (e.g. -checkaction=C -checkaction=context)

Comment on lines +1 to +3
#!/usr/bin/env bash

$DMD -c -o- -main -m${MODEL} ${EXTRA_FILES}/minimal/object.d
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed, every test in this test suite is compiled with -m${MODEL}

Comment on lines +1 to +8
#!/usr/bin/env bash

errmsg="$(! $DMD -c -o- -main -m32 -m64 ${EXTRA_FILES}/minimal/object.d 2>&1 > /dev/null)"
expected="Error: Conflicting target architectures specified: -m32 and -m64."

if [ "$errmsg" != "$expected" ]; then exit 1; fi

exit 0
Copy link
Contributor

@MoonlightSentinel MoonlightSentinel Apr 25, 2020

Choose a reason for hiding this comment

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

Avoid bash tests, either convert this into a dshell test or create fail_compilation/fail13292.d with an appropriate TEST_OUTPUT section, e.g.

/*
REQUIRED_ARGS: -m32 -m64
TEST_OUTPUT:
----
Error: Conflicting target architectures specified: $?:32mscoff=-m32mscoff and -m32|32=-m32 and -m64|64=-m64 and -m32$.
----
*/

@MoonlightSentinel
Copy link
Contributor

I did not expect destroying other tests with this change... I'll have a look on it later.

E.g. compilable/b18197.d relies on the ability to override the target architecture (which defaults to the hosts) because the bug is -m32(mscoff) specific.

@wolframw
Copy link
Contributor Author

I don't think we should do this because it's pretty common to allow flags to override previous flags. E.g. gcc allows both -m32 -m64 and other dmd flags can be specified mutliple times as well (e.g. -checkaction=C -checkaction=context)

Do you think it's better changing the error message to a warning or just scratching this whole thing?
(This issue is tagged "bootcamp", I sort of—perhaps naïvely so—assumed this to have a similar meaning to "preapproved".)

@MoonlightSentinel
Copy link
Contributor

Do you think it's better changing the error message to a warning or just scratching this whole thing?

Personally the latter because current behaviour is pretty common but let's see what other people think.

(This issue is tagged "bootcamp", I sort of—perhaps naïvely so—assumed this to have a similar meaning to "preapproved".)

To quote the wiki:

The "bootcamp" bugs are manually earmarked by senior contributors as bugs that are fit for a beginner to tackle. The ideal bootcamp bug is easy but not too easy, has good impact, can be fixed in 1-3 days by a motivated beginner, and offers a good learning experience.

@wolframw
Copy link
Contributor Author

"earmarked by senior contributors" and "has good impact" is exactly what makes me assume that the fix to such an issue has at least some value and will not end up in the bin (which is what I meant with "bootcamp" being similar to "preapproved").
Nonetheless, I now agree that this isn't a good idea. Any tests that have their own -m are pretty much unfixable, as far as I understand, which is in itself a good example why overriding the -m flag can be useful. Thanks for the helpful comments anyway.

@wolframw wolframw closed this Apr 28, 2020
@wolframw wolframw deleted the fix_13292 branch April 28, 2020 23:38
@Geod24
Copy link
Member

Geod24 commented Apr 28, 2020

@wolframw : Thanks for tackling this, and sorry it didn't end up being something we can merge.
I incorporated the discussion in this PR in the issue and marked it as WONTFIX.

For reference, anyone can do triage on Bugzilla, so we might have slightly different interpretation of what it means depending on when the label was applied / who applied it. I just did a quick search and some of those issues are definitely not trivial and require deep understanding of how things work, while some other seem much simpler.

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.

4 participants