[llvm-objcopy] Improve help messages #82830
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using spr 1.3.4
|
@llvm/pr-subscribers-llvm-binary-utilities Author: Fangrui Song (MaskRay) Changeshttps://reviews.llvm.org/D63820 added Sync the help messages to incorporate the CommandGuide improvement. Full diff: https://github.com/llvm/llvm-project/pull/82830.diff 3 Files Affected:
diff --git a/llvm/docs/CommandGuide/llvm-objcopy.rst b/llvm/docs/CommandGuide/llvm-objcopy.rst
index 0fb3c4bed643a5..9559fc3d86c4f8 100644
--- a/llvm/docs/CommandGuide/llvm-objcopy.rst
+++ b/llvm/docs/CommandGuide/llvm-objcopy.rst
@@ -450,7 +450,7 @@ them.
Set the type of section ``<section>`` to the integer ``<type>``. Can be
specified multiple times to update multiple sections.
-.. option:: --set-start-addr <addr>
+.. option:: --set-start <addr>
Set the start address of the output to ``<addr>``. Overrides any previously
specified :option:`--change-start` or :option:`--adjust-start` options.
diff --git a/llvm/tools/llvm-objcopy/CommonOpts.td b/llvm/tools/llvm-objcopy/CommonOpts.td
index 4222532a1a38f7..d41ccd953f516d 100644
--- a/llvm/tools/llvm-objcopy/CommonOpts.td
+++ b/llvm/tools/llvm-objcopy/CommonOpts.td
@@ -7,6 +7,9 @@ multiclass Eq<string name, string help> {
HelpText<help>;
}
+def grp_coff : OptionGroup<"kind">, HelpText<"OPTIONS (COFF specific)">;
+def grp_macho : OptionGroup<"kind">, HelpText<"OPTIONS (Mach-O specific)">;
+
def help : Flag<["--"], "help">;
def h : Flag<["-"], "h">, Alias<help>;
@@ -39,13 +42,13 @@ def p : Flag<["-"], "p">,
HelpText<"Alias for --preserve-dates">;
def strip_all : Flag<["--"], "strip-all">,
- HelpText<"Remove non-allocated sections outside segments. "
- ".gnu.warning* and .ARM.attribute sections are not "
- "removed">;
+ HelpText<"For ELF, remove all symbols and non-alloc sections not "
+ "segments, except for .gnu.warning*, .ARM.attribute, and the section name table. "
+ "For COFF and Mach-O, remove all symbols, debug sections, and relocations">;
def strip_all_gnu
: Flag<["--"], "strip-all-gnu">,
- HelpText<"Compatible with GNU's --strip-all">;
+ HelpText<"Remove all symbols, debug sections and relocations. Compatible with GNU's --strip-all">;
def strip_debug : Flag<["--"], "strip-debug">,
HelpText<"Remove all debug sections">;
@@ -64,7 +67,7 @@ def R : JoinedOrSeparate<["-"], "R">,
def strip_sections
: Flag<["--"], "strip-sections">,
- HelpText<"Remove all section headers and all sections not in segments">;
+ HelpText<"Remove all section headers and all section data not within segments">;
defm strip_symbol : Eq<"strip-symbol", "Strip <symbol>">,
MetaVarName<"symbol">;
@@ -75,17 +78,17 @@ def N : JoinedOrSeparate<["-"], "N">,
defm keep_section : Eq<"keep-section", "Keep <section>">,
MetaVarName<"section">;
-defm keep_symbol : Eq<"keep-symbol", "Do not remove symbol <symbol>">,
+defm keep_symbol : Eq<"keep-symbol", "When removing symbols, do not remove <symbol>">,
MetaVarName<"symbol">;
def K : JoinedOrSeparate<["-"], "K">,
Alias<keep_symbol>,
HelpText<"Alias for --keep-symbol">;
def keep_file_symbols : Flag<["--"], "keep-file-symbols">,
- HelpText<"Do not remove file symbols">;
+ HelpText<"Keep symbols of type STT_FILE, even if they would otherwise be stripped">;
def keep_undefined : Flag<["--"], "keep-undefined">,
- HelpText<"Do not remove undefined symbols">;
+ HelpText<"Do not remove undefined symbols">, Group<grp_macho>;
def only_keep_debug
: Flag<["--"], "only-keep-debug">,
@@ -94,16 +97,16 @@ def only_keep_debug
"sections useful for debugging purposes">;
def discard_locals : Flag<["--"], "discard-locals">,
- HelpText<"Remove compiler-generated local symbols, (e.g. "
- "symbols starting with .L)">;
+ HelpText<"Remove local symbols starting with .L">;
def X : Flag<["-"], "X">,
Alias<discard_locals>,
HelpText<"Alias for --discard-locals">;
def discard_all
: Flag<["--"], "discard-all">,
- HelpText<"Remove all local symbols except file and section symbols. Also "
- "remove all debug sections">;
+ HelpText<"Remove most local symbols. Different file formats may limit this to a subset. "
+ "For ELF, file and section symbols are not discarded. "
+ "Additionally, remove all debug sections">;
def x : Flag<["-"], "x">,
Alias<discard_all>,
HelpText<"Alias for --discard-all">;
diff --git a/llvm/tools/llvm-objcopy/ObjcopyOpts.td b/llvm/tools/llvm-objcopy/ObjcopyOpts.td
index bd041fabbdd7ac..0b0cab0abaf2cb 100644
--- a/llvm/tools/llvm-objcopy/ObjcopyOpts.td
+++ b/llvm/tools/llvm-objcopy/ObjcopyOpts.td
@@ -6,28 +6,25 @@ def B : JoinedOrSeparate<["-"], "B">,
Alias<binary_architecture>,
HelpText<"Alias for --binary-architecture">;
-defm target : Eq<"target", "Format of the input and output file">,
+defm target : Eq<"target", "Equivalent to --input-target and --output-target for the specified format">,
Values<"binary">;
def F : JoinedOrSeparate<["-"], "F">,
Alias<target>,
HelpText<"Alias for --target">;
-defm input_target : Eq<"input-target", "Format of the input file">,
- Values<"binary">;
+defm input_target : Eq<"input-target", "Read the input as the specified format">, MetaVarName<"format">;
def I : JoinedOrSeparate<["-"], "I">,
Alias<input_target>,
HelpText<"Alias for --input-target">;
-defm output_target : Eq<"output-target", "Format of the output file">,
- Values<"binary">;
+defm output_target : Eq<"output-target", "Write the output as the specified format">, MetaVarName<"format">;
def O : JoinedOrSeparate<["-"], "O">,
Alias<output_target>,
HelpText<"Alias for --output-target">;
-defm new_symbol_visibility : Eq<"new-symbol-visibility", "Visibility of "
- "symbols generated for binary input or added"
- " with --add-symbol unless otherwise"
- " specified. The default value is 'default'">;
+defm new_symbol_visibility
+ : Eq<"new-symbol-visibility", "Specirty the visibility of symbols automatically "
+ "created when using binary input or --add-symbol. The default is 'default'">;
def compress_debug_sections
: Joined<["--"], "compress-debug-sections=">,
@@ -39,8 +36,8 @@ def : Flag<["--"], "compress-debug-sections">, Alias<compress_debug_sections>,
def decompress_debug_sections : Flag<["--"], "decompress-debug-sections">,
HelpText<"Decompress DWARF debug sections">;
defm split_dwo
- : Eq<"split-dwo", "Equivalent to extract-dwo on the input file to "
- "<dwo-file>, then strip-dwo on the input file">,
+ : Eq<"split-dwo", "Equivalent to --extract-dwo and <dwo-file> as the output file and no other options, "
+ "and then --strip-dwo on the input file">,
MetaVarName<"dwo-file">;
defm add_gnu_debuglink
@@ -49,17 +46,15 @@ defm add_gnu_debuglink
defm rename_section
: Eq<"rename-section",
- "Renames a section from old to new, optionally with specified flags. "
- "Flags supported for GNU compatibility: alloc, load, noload, "
- "readonly, exclude, debug, code, data, rom, share, contents, merge, "
- "strings, large">,
+ "Rename sections called <old> to <new>, and apply any specified <flag> values. "
+ "See --set-section-flags for a list of supported flags">,
MetaVarName<"old=new[,flag1,...]">;
defm redefine_symbol
: Eq<"redefine-sym", "Change the name of a symbol old to new">,
MetaVarName<"old=new">;
defm redefine_symbols
: Eq<"redefine-syms",
- "Reads a list of symbol pairs from <filename> and runs as if "
+ "Read a list of symbol pairs from <filename> and runs as if "
"--redefine-sym=<old>=<new> is set for each one. <filename> "
"contains two symbols per line separated with whitespace and may "
"contain comments beginning with '#'. Leading and trailing "
@@ -74,7 +69,7 @@ def j : JoinedOrSeparate<["-"], "j">,
HelpText<"Alias for --only-section">;
defm add_section
: Eq<"add-section",
- "Make a section named <section> with the contents of <file>">,
+ "Add a section named <section> with the contents of <file>">,
MetaVarName<"section=file">;
defm set_section_alignment
@@ -83,8 +78,8 @@ defm set_section_alignment
defm set_section_flags
: Eq<"set-section-flags",
- "Set section flags for a given section. Flags supported for GNU "
- "compatibility: alloc, load, noload, readonly, exclude, debug, code, "
+ "Set section properties based on the specified <flags>. Supported flag names are: "
+ "alloc, load, noload, readonly, exclude, debug, code, "
"data, rom, share, contents, merge, strings, large">,
MetaVarName<"section=flag1[,flag2,...]">;
@@ -97,24 +92,25 @@ def S : Flag<["-"], "S">,
Alias<strip_all>,
HelpText<"Alias for --strip-all">;
def strip_dwo : Flag<["--"], "strip-dwo">,
- HelpText<"Remove all DWARF .dwo sections from file">;
+ HelpText<"Remove all DWARF .dwo sections">;
def strip_non_alloc
: Flag<["--"], "strip-non-alloc">,
- HelpText<"Remove all non-allocated sections outside segments">;
+ HelpText<"Remove all non-allocated sections that are not within segments">;
defm strip_unneeded_symbol
: Eq<"strip-unneeded-symbol",
- "Remove symbol <symbol> if it is not needed by relocations">,
+ "Remove all symbols named <symbol> that are local or undefined and "
+ "are not required by any relocation">,
MetaVarName<"symbol">;
defm strip_unneeded_symbols
: Eq<"strip-unneeded-symbols",
- "Reads a list of symbols from <filename> and removes them "
- "if they are not needed by relocations">,
+ "Remove all symbols whose names appear in the file <file>, if they "
+ "are local or undefined and are not required by any relocation">,
MetaVarName<"filename">;
defm subsystem
: Eq<"subsystem",
- "Set PE subsystem and version">,
- MetaVarName<"name[:version]">;
+ "Set the PE subsystem, optionally subsystem version">,
+ MetaVarName<"name[:version]">, Group<grp_coff>;
def extract_dwo
: Flag<["--"], "extract-dwo">,
@@ -132,11 +128,13 @@ def localize_hidden
: Flag<["--"], "localize-hidden">,
HelpText<
"Mark all symbols that have hidden or internal visibility as local">;
-defm localize_symbol : Eq<"localize-symbol", "Mark <symbol> as local">,
- MetaVarName<"symbol">;
+defm localize_symbol
+ : Eq<"localize-symbol", "Mark any defined non-common symbol named <symbol> as local">,
+ MetaVarName<"symbol">;
defm localize_symbols
: Eq<"localize-symbols",
- "Reads a list of symbols from <filename> and marks them local">,
+ "Read a list of names from <filename> and mark any defined non-common "
+ "symbols with those names as local">,
MetaVarName<"filename">;
def L : JoinedOrSeparate<["-"], "L">,
@@ -148,13 +146,14 @@ defm globalize_symbol : Eq<"globalize-symbol", "Mark <symbol> as global">,
defm globalize_symbols
: Eq<"globalize-symbols",
- "Reads a list of symbols from <filename> and marks them global">,
+ "Read a list of symbols from <filename> and mark defined symbols"
+ " with those names as global">,
MetaVarName<"filename">;
defm keep_global_symbol
: Eq<"keep-global-symbol",
- "Convert all symbols except <symbol> to local. May be repeated to "
- "convert all except a set of symbols to local">,
+ "Mark all symbols local, except for symbols with the name <symbol>. "
+ "Can be specified multiple times to ignore multiple symbols">,
MetaVarName<"symbol">;
def G : JoinedOrSeparate<["-"], "G">,
Alias<keep_global_symbol>,
@@ -162,34 +161,34 @@ def G : JoinedOrSeparate<["-"], "G">,
defm keep_global_symbols
: Eq<"keep-global-symbols",
- "Reads a list of symbols from <filename> and runs as if "
+ "Read a list of symbols from <filename> and run as if "
"--keep-global-symbol=<symbol> is set for each one. <filename> "
"contains one symbol per line and may contain comments beginning with "
"'#'. Leading and trailing whitespace is stripped from each line. May "
"be repeated to read symbols from many files">,
MetaVarName<"filename">;
-defm weaken_symbol : Eq<"weaken-symbol", "Mark <symbol> as weak">,
+defm weaken_symbol : Eq<"weaken-symbol", "Mark any global symbol named <symbol> as weak">,
MetaVarName<"symbol">;
defm weaken_symbols
: Eq<"weaken-symbols",
- "Reads a list of symbols from <filename> and marks them weak">,
+ "Read a list of symbols from <filename> and mark global symbols with those names as weak">,
MetaVarName<"filename">;
def W : JoinedOrSeparate<["-"], "W">,
Alias<weaken_symbol>,
HelpText<"Alias for --weaken-symbol">;
def weaken : Flag<["--"], "weaken">,
- HelpText<"Mark all global symbols as weak">;
+ HelpText<"Mark all defined global symbols as weak">;
defm strip_symbols
: Eq<"strip-symbols",
- "Reads a list of symbols from <filename> and removes them">,
+ "Remove all symbols whose names appear in the file <filename>">,
MetaVarName<"filename">;
defm keep_symbols
: Eq<"keep-symbols",
- "Reads a list of symbols from <filename> and runs as if "
+ "Read a list of symbols from <filename> and runs as if "
"--keep-symbol=<symbol> is set for each one. <filename> "
"contains one symbol per line and may contain comments beginning with "
"'#'. Leading and trailing whitespace is stripped from each line. May "
|
emaste
reviewed
Feb 23, 2024
| "--keep-global-symbol=<symbol> is set for each one. <filename> " | ||
| "contains one symbol per line and may contain comments beginning with " | ||
| "'#'. Leading and trailing whitespace is stripped from each line. May " | ||
| "be repeated to read symbols from many files">, | ||
| MetaVarName<"filename">; | ||
|
|
||
| defm weaken_symbol : Eq<"weaken-symbol", "Mark <symbol> as weak">, | ||
| defm weaken_symbol : Eq<"weaken-symbol", "Mark any global symbol named <symbol> as weak">, |
There was a problem hiding this comment.
is there something specific we want to capture with the explicit language here? Is that we're trying to capture the idea that there may be zero, one, or many instances of the symbol?
There was a problem hiding this comment.
https://llvm.org/docs/CommandGuide/llvm-objcopy.html mentions "Can be specified multiple times..." many times. It's useful to be preciser at the cost of a few more words "Can be specified multiple times".
For help messages, however, I feel that we probably should omit them for brevity, as all options can be specified multiple times, and the behavior is somewhat well-known.
There was a problem hiding this comment.
Right; it's the "any" that seems odd to me. I.e., why not "Mark global symbol named " or "Mark the global symbol named "
There was a problem hiding this comment.
Thanks for the suggestion. Let me use "Mark global symbols named "
emaste
reviewed
Feb 23, 2024
| MetaVarName<"filename">; | ||
|
|
||
| defm subsystem | ||
| : Eq<"subsystem", | ||
| "Set PE subsystem and version">, | ||
| MetaVarName<"name[:version]">; | ||
| "Set the PE subsystem, optionally subsystem version">, |
There was a problem hiding this comment.
Set the PE subsystem and optionally the subsystem version?
(Although I think it's clear from "name[:version]" that the version is optional.)
There was a problem hiding this comment.
llvm-objcopy.rst uses "Set the PE subsystem, and optionally subsystem version.". Let me just use its version.
emaste
reviewed
Feb 23, 2024
|
Looks good overall, a couple of comments. I'm a bit puzzled by the distinction between referring to a singular symbol named and "all" symbols named . |
Created using spr 1.3.4
jh7370
reviewed
Feb 26, 2024
kuilpd
reviewed
Feb 26, 2024
| MetaVarName<"old=new[,flag1,...]">; | ||
| defm redefine_symbol | ||
| : Eq<"redefine-sym", "Change the name of a symbol old to new">, | ||
| MetaVarName<"old=new">; | ||
| defm redefine_symbols | ||
| : Eq<"redefine-syms", | ||
| "Reads a list of symbol pairs from <filename> and runs as if " | ||
| "Read a list of symbol pairs from <filename> and runs as if " |
There was a problem hiding this comment.
Suggested change
| "Read a list of symbol pairs from <filename> and runs as if " | |
| "Read a list of symbol pairs from <filename> and run as if " |
kuilpd
reviewed
Feb 26, 2024
| MetaVarName<"filename">; | ||
|
|
||
| defm keep_symbols | ||
| : Eq<"keep-symbols", | ||
| "Reads a list of symbols from <filename> and runs as if " | ||
| "Read a list of symbols from <filename> and runs as if " |
There was a problem hiding this comment.
Suggested change
| "Read a list of symbols from <filename> and runs as if " | |
| "Read a list of symbols from <filename> and run as if " |
kuilpd
added a commit
to kuilpd/llvm-project
that referenced
this pull request
Feb 26, 2024
Created using spr 1.3.4
|
Thanks for the suggestions. Applied. |
emaste
approved these changes
Feb 27, 2024
MaskRay
deleted the
users/MaskRay/spr/llvm-objcopy-improve-help-messages
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://reviews.llvm.org/D63820 added
llvm/docs/CommandGuide/llvm-objcopy.rst with clearer semantics, e.g.
Sync the help messages to incorporate the CommandGuide improvement.
While here, switch to the conventional imperative sentences for a few
options. Additionally, mark some options as grp_coff or grp_macho.