This is an archive of the discontinued LLVM Phabricator instance.

[ms] [llvm-ml] Allow the /Zs parameter as a synonym for -filetype=null
ClosedPublic

Authored by epastor on Oct 23 2020, 10:52 AM.

Details

Summary

For ml.exe, /Zs implies a syntax check with no output files.

Diff Detail

Event Timeline

epastor created this revision.Oct 23 2020, 10:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 23 2020, 10:52 AM
epastor requested review of this revision.Oct 23 2020, 10:52 AM
epastor updated this revision to Diff 303176.Nov 5 2020, 10:33 AM

Rebase on HEAD

epastor updated this revision to Diff 303177.Nov 5 2020, 10:33 AM

Rebase on HEAD

epastor updated this revision to Diff 303187.Nov 5 2020, 10:44 AM

Rebase on parent

thakis added a subscriber: thakis.Nov 6 2020, 6:36 AM

The unrelated change looks a) unrelated b) incorrect.

No test?

Can we drop -filetype=nul now that we have this?

llvm/tools/llvm-ml/llvm-ml.cpp
77

mention this change in cl description, add test for it

....but I don't think this change is correct?

C:\src>ml64 foo.asm /c /foasdf.obj /nologo
Microsoft (R) Macro Assembler (x64) Version 14.25.28614.0
Copyright (C) Microsoft Corporation.  All rights reserved.

 Assembling: foo.asm
MASM : warning A4018:invalid command-line option : /foasdf.obj
thakis requested changes to this revision.Nov 6 2020, 6:36 AM
This revision now requires changes to proceed.Nov 6 2020, 6:36 AM
epastor updated this revision to Diff 307165.Nov 23 2020, 12:10 PM

Rebase on parent

epastor updated this revision to Diff 307169.Nov 23 2020, 12:19 PM

Rebase on parent

epastor added inline comments.
llvm/tools/llvm-ml/llvm-ml.cpp
77

This is correcting us to match ml64; we previously considered /fo equivalent to /Fo, but this was a mistake. However, I'll push it up the stack to the change that introduced the problem instead.

epastor updated this revision to Diff 307188.Nov 23 2020, 1:32 PM

Rebase on parent

epastor updated this revision to Diff 307691.Nov 25 2020, 12:35 PM

Rebase on parent

epastor marked an inline comment as done.Nov 25 2020, 12:39 PM
epastor updated this revision to Diff 308783.Dec 1 2020, 2:48 PM

Rebase on parent

epastor added a reviewer: rnk.Dec 1 2020, 3:06 PM
thakis requested changes to this revision.Dec 3 2020, 7:02 AM

Still no test, as far as I can tell. I think the suggestion below turns this into a 3-line diff (+ test).

llvm/tools/llvm-ml/Opts.td
50

Instead of all the DerivedArgList changes below, doesn't just saying ..., Alias<filetype>, AliasArgs<"none"> here do the same thing?

This revision now requires changes to proceed.Dec 3 2020, 7:02 AM
epastor updated this revision to Diff 310884.Dec 10 2020, 7:17 AM

Use AliasArgs to simplify

epastor updated this revision to Diff 310889.Dec 10 2020, 7:37 AM

Fix errors in Alias/AliasArgs use, and add tests

Still no test, as far as I can tell. I think the suggestion below turns this into a 3-line diff (+ test).

Tests added, thanks.

llvm/tools/llvm-ml/Opts.td
50

... yes, and I didn't know that was an option. Fixed - though it did require me to move these below the LLVM-style args, so we could successfully alias it to one of them.

Thanks, this is much better!

thakis accepted this revision.Dec 11 2020, 7:00 AM

v nice :)

This revision is now accepted and ready to land.Dec 11 2020, 7:00 AM
This revision was landed with ongoing or failed builds.Mar 17 2021, 9:19 AM
This revision was automatically updated to reflect the committed changes.