Page MenuHomePhabricator

LLVM Driver Multicall tool
Needs ReviewPublic

Authored by beanz on Sep 17 2021, 9:36 AM.

Details

Summary

This patch adds an llvm-driver multicall tool that can combine multiple
LLVM-based tools. The build infrastructure is enabled for a tool by
adding the GENERATE_DRIVER option to the add_llvm_executable CMake
call, and changing the tool's main function to a canonicalized
tool_name_main format (i.e. llvm_ar_main, clang_main, etc...).

As currently implemented llvm-driver contains dsymutil, llvm-ar,
llvm-cxxfilt, llvm-objcopy, and clang (if clang is included in the
build).

llvm-driver can be disabled from builds by setting
LLVM_TOOL_LLVM_DRIVER_BUILD=Off.

There are several limitations in the current implementation, which can
be addressed in subsequent patches:

(1) the multicall binary cannot currently properly handle
multi-dispatch tools. This means symlinking llvm-ranlib to llvm-driver
will not properly result in llvm-ar's main being called.
(2) the multicall binary cannot be comprised of tools containing
conflicting cl::opt options as the global cl::opt option list cannot
contain duplicates.

These limitations can be addressed in subsequent patches.

Diff Detail

Event Timeline

beanz created this revision.Sep 17 2021, 9:36 AM
beanz requested review of this revision.Sep 17 2021, 9:36 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 17 2021, 9:36 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aganea added a subscriber: aganea.Sep 17 2021, 9:41 AM

That's pretty nice! Have you thought about looking into a lit option (triggered by a cmake flag maybe) that would change the substitution for the tools that are enabled by llvm-driver to use the latter instead when running the tests?

phosek added inline comments.Sep 17 2021, 10:23 AM
llvm/tools/llvm-driver/CMakeLists.txt
18

As was already suggested on D104686, I'd prefer naming this just llvm so you can invoke tools with llvm name which doesn't require any additional typing compared to llvm-name.

That's pretty nice! Have you thought about looking into a lit option (triggered by a cmake flag maybe) that would change the substitution for the tools that are enabled by llvm-driver to use the latter instead when running the tests?

That's an awesome idea. I had been thinking about having a CMake option to generate symlinks instead of the tools, but you could totally bake this into the lit substitutions. I'll take a look at that.

llvm/tools/llvm-driver/CMakeLists.txt
18

Can do

beanz updated this revision to Diff 373300.Sep 17 2021, 12:04 PM

Renaming llvm-driver binary to llvm.

This is really a great change, thanks @beanz!

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
404

Shouldn't we say:

int objcopy_main(int argc, char **argv) {

here and the other places + all supporting code, if we want llvm objcopy (without the dash) like @phosek suggests?

beanz added inline comments.Sep 17 2021, 2:51 PM
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
404

I had a different thought for that. I think we want the tools to respond to llvm-objcopy since we will want them to exist in parallel to binutils tools just like the current tools do today.

Many of the current tools also support symlink-redirection, to support that we'll need to have a multiplex where multiple tool names point to the same main function.

Handling that was my point (1) in the main commit message, and I intended to work on it in a follow-on commit.

@beanz Is this ready to land or do you plan on making any more changes?

Sorry for disappearing on this. I'm working right now on adding support for multi-entry tools (like objcopy). I'll get an update in today.

beanz updated this revision to Diff 404114.Jan 28 2022, 11:37 AM

Adding support for multiple subcommands mapping to the same entry.

Hope that someone familiar with CMake can take a look.

As currently implemented llvm-driver contains dsymutil, llvm-ar, llvm-cxxfilt, llvm-objcopy, and clang (if clang is included in the build).

I think either (bundled clang+binary utilities) or (bundled lld+binary utilities) is fine.
Some folks are interested in PGO builds for clang and lld. Not bundling clang and lld together can make both optimized.
The performance of binary utilities likely does not matter that much, so bundling them with either clang or lld should be fine.


I have tried the latest diff but -DLLVM_ENABLE_PROJECTS='clang;clang-tools-extra;flang;lldb;lld;compiler-rt;openmp;mlir;cross-project-tests' -DLLVM_TOOL_LLVM_DRIVER_BUILD=on gives me:

CMake Error at cmake/modules/LLVM-Config.cmake:110 (target_link_libraries):                 
  The keyword signature for target_link_libraries has already been used with                
  the target "obj.dsymutil".  All uses of target_link_libraries with a target               
  must be either all-keyword or all-plain.                                                  
                                                                                            
  The uses of the keyword signature are here:                                                                                                 
                                                                                            
   * cmake/modules/AddLLVM.cmake:897 (target_link_libraries)                                
                                                                                            
Call Stack (most recent call first):                                                        
  cmake/modules/LLVM-Config.cmake:95 (explicit_llvm_config)                                 
  cmake/modules/AddLLVM.cmake:898 (llvm_config)                                                                                               
  cmake/modules/AddLLVM.cmake:1264 (add_llvm_executable)                                                                                      
  tools/dsymutil/CMakeLists.txt:21 (add_llvm_tool)
MaskRay added a comment.EditedMar 12 2022, 10:36 AM

I have tried the latest diff but -DLLVM_ENABLE_PROJECTS='clang;clang-tools-extra;flang;lldb;lld;compiler-rt;openmp;mlir;cross-project-tests' -DLLVM_TOOL_LLVM_DRIVER_BUILD=on gives me:

CMake Error at cmake/modules/LLVM-Config.cmake:110 (target_link_libraries):                 
  The keyword signature for target_link_libraries has already been used with                
  the target "obj.dsymutil".  All uses of target_link_libraries with a target               
  must be either all-keyword or all-plain.                                                  
                                                                                            
  The uses of the keyword signature are here:                                                                                                 
                                                                                            
   * cmake/modules/AddLLVM.cmake:897 (target_link_libraries)                                
                                                                                            
Call Stack (most recent call first):                                                        
  cmake/modules/LLVM-Config.cmake:95 (explicit_llvm_config)                                 
  cmake/modules/AddLLVM.cmake:898 (llvm_config)                                                                                               
  cmake/modules/AddLLVM.cmake:1264 (add_llvm_executable)                                                                                      
  tools/dsymutil/CMakeLists.txt:21 (add_llvm_tool)

@beanz ⬆️

Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2022, 10:36 AM

Another potential future improvement is error reporting for subcommands:

$ ./bin/llvm clang       
llvm: error: no input files
$ ./bin/clang    
clang-15: error: no input files

Ideally, the multicall tool would produce the same error message.

llvm/cmake/modules/AddLLVM.cmake
889

The error that @MaskRay reported is because we use PUBLIC with target_link_libraries() here, but we use plain form of target_link_libraries() later in explicit_llvm_config that is invoked llvm_config (see https://github.com/llvm/llvm-project/blob/1643f01232b41b93e5f3a21d89b111efab0e378a/llvm/cmake/modules/LLVM-Config.cmake#L110). Omitting PUBLIC here appears to be sufficient to avoid the error.

llvm/tools/CMakeLists.txt
58

Should this be guarded by LLVM_TOOL_LLVM_DRIVER_BUILD to behave as the commit message says? Currently the llvm-driver appears to be built regardless of LLVM_TOOL_LLVM_DRIVER_BUILD.

llvm/tools/llvm-driver/llvm-driver.cpp
44

When the tool is invoked without any arguments (that is, simply as ./bin/llvm) this will lead to out-of-bounds array access. We should handle this case explicitly.

@beanz Let me know if you need help, I'm happy to commandeer the change if you're too busy.

beanz added a comment.Mar 24 2022, 9:32 AM

@beanz Let me know if you need help, I'm happy to commandeer the change if you're too busy.

Please feel free to commandeer it. I'm really sorry I've held this up so long and I'm totally swamped right now :(.