Page MenuHomePhabricator

[ms] [llvm-ml] Add placeholder for llvm-ml, based on llvm-mc
ClosedPublic

Authored by epastor on Jan 13 2020, 8:34 PM.

Details

Summary

As discussed on the mailing list, I plan to introduce an ml-compatible MASM assembler as part of providing more of the Windows build tools. This will be similar to llvm-mc, but with different command-line parameters.

This placeholder is purely a stripped-down version of llvm-mc; we'll eventually add support for the Microsoft-style command-line flags, and back it with a MASM parser.

Diff Detail

Event Timeline

epastor created this revision.Jan 13 2020, 8:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2020, 8:34 PM

Unit tests: pass. 61799 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Nice! Looks like a good start.

It might be nice to add a test that just runs llvm-ml --help to check that the binary runs.

I'd imagine that once this is mostly done, we want to busy-box into clang-cl so that clang-cl file.asm runs the ml-style asm parser by default, and we'd have some -fasm-mode flag to override this (so that .s files can be parsed in ml mode and .asm files in gas mode, if desired). So far, this is all compatible with that, but something to keep in mind going forward.

llvm/tools/llvm-ml/CMakeLists.txt
2

You should probably add this in this commit as well :)

llvm/tools/llvm-ml/Disassembler.h
11

Is this comment up-to-date? Should probably mention ml?

llvm/tools/llvm-ml/llvm-ml.cpp
102

If you intend to add these soon, add "// FIXME: Enable once $condition", else remove commented-out code for now

125

Even more important than this flag is -fdebug-compilation-dir (ctrl-f "fdebug-compilation" in http://blog.llvm.org/2019/11/deterministic-builds-with-clang-and-lld.html )

211

Hm!

Elaborate, or remove

epastor updated this revision to Diff 238288.Jan 15 2020, 9:10 AM
epastor marked 5 inline comments as done.

Fixing most feedback; basic functionality test still pending.

epastor added inline comments.Jan 15 2020, 9:13 AM
llvm/tools/llvm-ml/Disassembler.h
11

This is accurate; Disassembler.h and Disassembler.cpp are identical to that in llvm-mc. The only difference is that when run as part of llvm-ml, it will be passed different parameters by default. Is there a way I can share the code between them instead?

llvm/tools/llvm-ml/llvm-ml.cpp
125

Already in place; see above.

211

Removed.

Unit tests: pass. 61799 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

epastor updated this revision to Diff 238332.Jan 15 2020, 12:38 PM

Add basic tests, confirming ENOENT for a non-existent file and that --help shows a usage message.

It might be nice to add a test that just runs llvm-ml --help to check that the binary runs.

Done.

I'd imagine that once this is mostly done, we want to busy-box into clang-cl so that clang-cl file.asm runs the ml-style asm parser by default, and we'd have some -fasm-mode flag to override this (so that .s files can be parsed in ml mode and .asm files in gas mode, if desired). So far, this is all compatible with that, but something to keep in mind going forward.

Something like that, once this is mostly done... though I'm not familiar with what you mean by "busy-box into clang-cl". I don't think that should be too tricky.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt

thakis accepted this revision.Jan 15 2020, 1:12 PM

Looks fine to me as a first step, with more uncommented code and a not very actionable FIXME removed :)

Maybe wait until EOD to see if rnk wants to chime in too.

llvm/tools/llvm-ml/llvm-ml.cpp
289

?

359

?

llvm/utils/gn/secondary/llvm/tools/llvm-ml/BUILD.gn
14

?

This revision is now accepted and ready to land.Jan 15 2020, 1:12 PM
epastor updated this revision to Diff 238354.Jan 15 2020, 1:27 PM

Fix an inconsistency apparently introduced in an earlier rebase.

epastor updated this revision to Diff 238357.Jan 15 2020, 1:32 PM

Removing some more commented-code artifacts from development

epastor marked 3 inline comments as done.Jan 15 2020, 1:33 PM

Unit tests: pass. 61801 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: pass. 61801 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

epastor updated this revision to Diff 238847.Jan 17 2020, 12:15 PM

Fix formatting where reasonable

Unit tests: pass. 61801 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

This revision was automatically updated to reflect the committed changes.

@epastor This is causing build bots failures, please can you take a look? http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/3121

@epastor This is causing build bots failures, please can you take a look? http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/3121

Looks real to me - this entire tool is premised on the availability of the (i386|x86_64)-pc-windows targets, since it's intended to imitate ml.exe & ml64.exe. Apparently at least one isn't available when building in this environment? Any reason for that?

Also, would there be a way to keep this tool from building (and exclude it from tests) if those targets aren't available?

Quick note: I've reverted this change in commit 0eeddf1ac59066567a096ad95344f43c38e6b04f due to the build breakages on all ARM platforms.

@RKSimon If you have any advice for relanding this safely, it'd be much appreciated!

Quick note: I've reverted this change in commit 0eeddf1ac59066567a096ad95344f43c38e6b04f due to the build breakages on all ARM platforms.

@RKSimon If you have any advice for relanding this safely, it'd be much appreciated!

Can you replicate the cmake settings in http://lab.llvm.org:8011/builders/llvm-clang-win-x-aarch64/builds/3114/steps/cmake-configure/logs/stdio ? The issue seems in part due to the available targets/triples:

llvm-ml.exe: error: : error: unable to get target for 'x86_64-pc-windows-msvc', see --version and --triple.
RKSimon reopened this revision.Jan 18 2020, 7:00 AM
This revision is now accepted and ready to land.Jan 18 2020, 7:00 AM
RKSimon requested changes to this revision.Jan 18 2020, 7:01 AM
This revision now requires changes to proceed.Jan 18 2020, 7:01 AM
epastor added a comment.EditedJan 18 2020, 10:35 AM

Quick note: I've reverted this change in commit 0eeddf1ac59066567a096ad95344f43c38e6b04f due to the build breakages on all ARM platforms.

@RKSimon If you have any advice for relanding this safely, it'd be much appreciated!

Can you replicate the cmake settings in http://lab.llvm.org:8011/builders/llvm-clang-win-x-aarch64/builds/3114/steps/cmake-configure/logs/stdio ? The issue seems in part due to the available targets/triples:

llvm-ml.exe: error: : error: unable to get target for 'x86_64-pc-windows-msvc', see --version and --triple.

As mentioned in my earlier comment - I think the issue is ENTIRELY due to the available targets/triples, since this tool forces to the (i386|x86_64)-pc-windows-msvc targets/triples. It's designed to (eventually) mimic Microsoft's ml.exe. I can try to broaden it, but I'm concerned about functionality - MASM is a pretty specific assembly dialect. No chance you know a standard way to set it up so this tool won't build unless those triples are available?

You might be able to fix it with the equivalent of rG80146fc13ada

epastor updated this revision to Diff 239023.Jan 19 2020, 8:31 PM

Tag non-trivial llvm-ml tests as requiring x86-registered-target, since llvm-ml's first draft only targets x86.

epastor updated this revision to Diff 239024.Jan 19 2020, 8:34 PM

Rebasing past the reversion

epastor added a comment.EditedJan 19 2020, 8:38 PM

Can you replicate the cmake settings in http://lab.llvm.org:8011/builders/llvm-clang-win-x-aarch64/builds/3114/steps/cmake-configure/logs/stdio ? The issue seems in part due to the available targets/triples:

llvm-ml.exe: error: : error: unable to get target for 'x86_64-pc-windows-msvc', see --version and --triple.

@RKSimon I replicated the issue as you suggested. With the added "REQUIRES" line, I can confirm it's correctly identified as unsupported. This shouldn't cause the same buildbot breakage anymore.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt

Unit tests: fail. 61977 tests passed, 1 failed and 783 were skipped.

failed: LLVM.Bindings/Go/go.test

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

RKSimon accepted this revision.Jan 20 2020, 12:22 AM

Thanks - you might want to fix the "newline at end of file" and clang-format warnings but other than that LGTM for re-commit. Cheers.

This revision is now accepted and ready to land.Jan 20 2020, 12:22 AM
epastor updated this revision to Diff 239110.Jan 20 2020, 6:17 AM

Fix missing newline at end of file

Unit tests: unknown.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt

This revision was automatically updated to reflect the committed changes.

Only supporting x86 targets for llvm-ml makes sense to me. As far as I know, msvc doesn't have an ml.exe for arm64 either (?).

llvm/test/tools/llvm-ml/basic.test
1

This is an ok way of doing this, but since all tests in this directory are going to need it, it's a bit nicer to instead have a file called lit.local.cfg in this directory that contains:

if not ('X86' in config.root.targets):
    # We need support for X86.
    config.unsupported = True

Then all the tests in this dir will only run if X86 is in LLVM_TARGETS_TO_BUILD.

Only supporting x86 targets for llvm-ml makes sense to me. As far as I know, msvc doesn't have an ml.exe for arm64 either (?).

That's correct. For arm/arm64, they have armasm.exe and armasm64.exe, which consumes assembly in the ARM RVCT armasm format (which is different from gas format). (And even more for the record, if some cares, there's a perl script called gas-preprocessor available, which can do on-the-fly conversion from gas to armasm format, for at least some subsets of cleaned up arm assembly out there.)

epastor added a comment.EditedJan 21 2020, 1:58 PM

@kamaub The build failures you see should have been fixed by the followup commit r6ccebe004446.

EDIT: On inspection, (for example) http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/43185 shows that you're now seeing a different error that appears unrelated?

@kamaub The build failures you see should have been fixed by the followup commit r6ccebe004446.

EDIT: On inspection, (for example) http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/43185 shows that you're now seeing a different error that appears unrelated?

Thank you, I have confirmed that this is true.