[PATCH] D72679: [ms] [llvm-ml] Add placeholder for llvm-ml, based on llvm-mc

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

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.



================
Comment at: llvm/tools/llvm-ml/CMakeLists.txt:1
+# FIXME
----------------
You should probably add this in this commit as well :)


================
Comment at: llvm/tools/llvm-ml/Disassembler.h:10
+// This class implements the disassembler of strings of bytes written in
+// hexadecimal, from standard input or from a file.
+//
----------------
Is this comment up-to-date? Should probably mention ml?


================
Comment at: llvm/tools/llvm-ml/llvm-ml.cpp:101
+//ArchName("arch", cl::desc("Target arch to assemble for, "
+                          //"see -version for available targets"));
+
----------------
If you intend to add these soon, add "// FIXME: Enable once $condition", else remove commented-out code for now


================
Comment at: llvm/tools/llvm-ml/llvm-ml.cpp:124
+static cl::list<std::string>
+DebugPrefixMap("fdebug-prefix-map",
+               cl::desc("Map file source paths in debug info"),
----------------
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 )


================
Comment at: llvm/tools/llvm-ml/llvm-ml.cpp:210
+
+  // XXX hm
+  std::unique_ptr<MCTargetAsmParser> TAP(
----------------
Hm!

Elaborate, or remove


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72679/new/

https://reviews.llvm.org/D72679