[llvm-objdump] Only reject -long-option in objdump mode

[MaskRay]

Repurpose

[jhenderson]

I was under the impression that we were going to do this for llvm-objdump, not just the GNU-named version?

Also, this won't work for my downstream repo, where the executable is named <prefix>-llvm-objdump.

llvm/test/tools/llvm-objdump/long-option-style.test

6–7

Seems to me like copying would allow Windows developers to test this too?

[MaskRay]

I was also of the opinion that "llvm-objdump" (argv[0]) and "objdump" (argv[0]) should reject single-dash -long-option.

However, @compnerd mentioned that on Mach-O platforms, -long-option may be used. Xcode uses the source code of llvm-objdump but it is not clear how they use the tool. They have patches and tests which are not upstreamed. If they provided a concrete tool name, I would like to allow --long-option when argv[0] matches that tool name (may be otool or something).

If we know how they use the tool, for example, if they always pass --macho, we can get rid of the alias -m.

[jhenderson]

In D76250#1929314, @MaskRay wrote:

I was also of the opinion that "llvm-objdump" (argv[0]) and "objdump" (argv[0]) should reject single-dash -long-option.

However, @compnerd mentioned that on Mach-O platforms, -long-option may be used. Xcode uses the source code of llvm-objdump but it is not clear how they use the tool. They have patches and tests which are not upstreamed. If they provided a concrete tool name, I would like to allow --long-option when argv[0] matches that tool name (may be otool or something).

If we know how they use the tool, for example, if they always pass --macho, we can get rid of the alias -m.

Hmm... I was under the impression we didn't have to pay much attention to downstream developers if it makes upstream more complicated (I've been on the other end of that point on a number of occasions...). We've got a concrete argument for not accepting single-dashed long options (it makes the code simpler and more compatible with GNU, by avoiding any ambiguity for short options), so it seems to me that there needs to be a very strong argument against this.

[compnerd]

@jhenderson - accepting GNU options in GNU mode makes sense. This tool is *not* GNU - objdump is the GNU driver, llvm-objdump is the LLVM driver and accepts SUN style arguments.

[jhenderson]

In D76250#1956517, @compnerd wrote:

@jhenderson - accepting GNU options in GNU mode makes sense. This tool is *not* GNU - objdump is the GNU driver, llvm-objdump is the LLVM driver and accepts SUN style arguments.

llvm-objdump doesn't have a GNU/LLVM mode distinction, and I'd prefer to avoid adding one based on tool names if possible because these sorts of things lead to various problems when people want to rename files. For example, in the current patch, if I rename my copy of objdump, which happens to really be llvm-objdump, to, say my-objdump, it will suddenly change behaviour (this isn't an imaginary case either, I am involved in a toolchain where prefixes on the name are commonplace). This is a very similar problem to the issue that led to https://bugs.llvm.org/show_bug.cgi?id=45271 for llvm-objcopy. Whilst in this case, the difference is whether or not to accept long options, starting a precedent will mean that other behaviours will be configured differently in the future no doubt.

[compnerd]

The precedent for behaviour based on the name is long standing and not being introduced here. In fact the archiver already does this. clang also does this (clang vs clang++). This is not at all uncommon I think. The target triple prefixed tools should be treated as the tool that they specify and implicitly passing -target ... which is derived from the prefix.

[jhenderson]

In D76250#1958357, @compnerd wrote:

The precedent for behaviour based on the name is long standing and not being introduced here. In fact the archiver already does this. clang also does this (clang vs clang++). This is not at all uncommon I think. The target triple prefixed tools should be treated as the tool that they specify and implicitly passing -target ... which is derived from the prefix.

I'm well aware that there's precedent for it, but I'm also aware that this precedent has caused problems, as already highlighted. It also leads to a plethora of tools in the build output, in order to test all the behaviours at runtime and (presumably) to simplify packaging/installation etc. These copies, whilst on some platforms (Windows) are not symlinks, leading to multiple copies of the same tool (with different names) that are several hundred MBs in size in the build output.

@compnerd - how do you propose resolving the ambiguity of multiple short options, grouped together versus a single-long option, if single-dash is allowed for long options too?

[dsanders]

Hmm... I was under the impression we didn't have to pay much attention to downstream developers if it makes upstream more complicated (I've been on the other end of that point on a number of occasions...).

I believe that's mostly about not worrying about API compatibility. It's downstream forks responsibility to fix their fork to account for changes. I think this goes a bit further than not worrying about downstream forks though as unless the fork diverges from upstream the end users of common OS's can be impacted too. For example, I expect not supporting single-dashed options in objdump limits the options /usr/bin/objdump accepts on macOS and may break things that use it. Whether that's a real problem or not I'm not really sure.

[compnerd]

@jhenderson Traditionally, the long options all must come after the short options. Doing a prefix match seems reasonable way to handle this. Honestly, all the arguments have been "we like the GNU options because that's what we are used to" rather than any real technical issue. I don't think that is a valid justification for breaking existing users of these options.

[jhenderson]

In D76250#1970669, @compnerd wrote:

@jhenderson Traditionally, the long options all must come after the short options.

Traditionally for who?

Honestly, all the arguments have been "we like the GNU options because that's what we are used to" rather than any real technical issue.

It's more like there's a straight-up use-case incompatibility here. For those trying to migrate their toolchain from GNU, they may run into breakages, due to the way llvm-objdump's option parsing works currently. Based on conversations I've had in the past at round tables etc, many people want a strict drop-in replacement, because the tweaking of configure scripts that often use these tools simply isn't possible.

Doing a prefix match seems reasonable way to handle this.

I highlighted in my other comments why a prefix match is problematic. It doesn't help every use-case (some people use prefixes on the tool name), and potentially (depending on whether we adjust build scripts like we have for other versions of this) bloats the build output.

Could we base the behaviour on the host OS instead of the tool name? If the OS is Mach-O, allow single-dash long options, otherwise don't?

llvm/test/tools/llvm-objdump/long-option-style.test

+## Don't make symlinks on Windows.
+# UNSUPPORTED: system-windows
+
+# RUN: rm -rf %t
+# RUN: mkdir %t
+# RUN: ln -s llvm-objdump %t/objdump
+# RUN: ln -s llvm-objdump %t/objdump-11
+
+# RUN: llvm-objdump --help
+# RUN: llvm-objdump -help
+
+## -long-option does not fit with grouped short options.
+## Disallow them in objdump mode.
+# RUN: %t/objdump --help
+# RUN: not %t/objdump -help 2>&1 | FileCheck --check-prefix=UNKNOWN %s
+
+## objdump may be suffixed by a version.
+# RUN: %t/objdump-11 --help
+# RUN: not %t/objdump-11 -help 2>&1 | FileCheck --check-prefix=UNKNOWN %s
+
+# UNKNOWN: Unknown command line argument '-help'.

llvm/tools/llvm-objdump/llvm-objdump.cpp

int main(int argc, char **argv) {
   // Initialize targets and assembly printers/parsers.
   InitializeAllTargetInfos();
   InitializeAllTargetMCs();
   InitializeAllDisassemblers();
 
   // Register the target printer for --version.
   cl::AddExtraVersionPrinter(TargetRegistry::printRegisteredTargetsForVersion);
 
+  // If called objdump, follow GNU long option style and reject -long-option.
+  StringRef Stem = sys::path::stem(argv[0]);
+  bool LongOptionsUseDoubleDash = Stem.startswith("objdump");
   cl::ParseCommandLineOptions(argc, argv, "llvm object file dumper\n", nullptr,
-                              /*EnvVar=*/nullptr,
-                              /*LongOptionsUseDoubleDash=*/true);
+                              /*EnvVar=*/nullptr, LongOptionsUseDoubleDash);
 
   if (StartAddress >= StopAddress)
     reportCmdLineError("start address should be less than stop address");
 
   ToolName = argv[0];
 
   // Defaults to a.out if no filenames specified.
   if (InputFilenames.empty())