[PATCH] D104686: [WIP][llvm] LLVM Busybox Prototype

Petr Hosek via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 22 15:05:28 PDT 2021


phosek added inline comments.


================
Comment at: llvm/CMakeLists.txt:434
 
+option(LLVM_ENABLE_EXPERIMENTAL_BUSYBOX "Enable usage of llvm tools through a single binary" OFF)
+
----------------
I think we should avoid the term busybox to avoid the confusion with https://busybox.net/, instead we should probably use the term multiplexing (or muxing for short) which has been also used by https://wdtz.org/files/oopsla18-allmux-dietz.pdf.

We could name the option as `LLVM_ENABLE_EXPERIMENTAL_MULTIPLEXING` (or `LLVM_ENABLE_EXPERIMENTAL_MUXING`)  and name the tool as `llvm-mux`.


================
Comment at: llvm/tools/llvm-busybox/Tools.def:5-13
+#ifndef OBJDUMP_TOOL
+#define OBJDUMP_TOOL(BusyboxName, LLVMName) \
+  TOOL(BusyboxName, LLVMName, llvm_objdump_main)
+#endif
+
+#ifndef OBJCOPY_TOOL
+#define OBJCOPY_TOOL(BusyboxName, LLVMName) \
----------------
I'd like to avoid centralized registry which I think would be difficult to scale and impossible to use for out-of-tree tools.

Instead, I'd be more interested in following an architecture similar to what we use for passes, so having `ToolRegistry` (akin to `PassRegistry`) with a single global instance that you could register each tool with similar to how `RegisterPass` is used.

We could consider requiring a class for each tool which would wrap the state (like the command line arguments) and then use it like:
```
static RegisterTool<ObjCopyTool> OT("objcopy", "Copies the contents of an object file to another");
```


================
Comment at: llvm/tools/llvm-busybox/Tools.def:15
+
+OBJDUMP_TOOL("objdump", "llvm-objdump")
+OBJDUMP_TOOL("otool", "llvm-otool")
----------------
Rather than duplicating the name, could we just construct `BusyboxName` from `LLVMName` by stripping the `llvm-` prefix if it exists?


================
Comment at: llvm/tools/llvm-busybox/main.cpp:135
+  size_t NewArgv0Len = ToolName.size();
+  std::unique_ptr<char> NewArgv0(new char[NewArgv0Len + 1]);
+  strncpy(NewArgv0.get(), ToolName.data(), NewArgv0Len);
----------------
Does this need to be heap allocated if it doesn't outlive this function? Could this just be a `SmallString`?


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:414-418
+#ifdef LLVM_ENABLE_EXPERIMENTAL_BUSYBOX
+int llvm_objcopy_main(int argc, char **argv) {
+#else
 int main(int argc, char **argv) {
+#endif
----------------
We could avoid the `ifdef` just by providing a macro that expands appropriately depending on the mode, something like:
```
LLVM_TOOL_MAIN(objcopy, int argc, char **argv)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104686



More information about the llvm-commits mailing list