Skip to content

Commit f44d013

Browse files
author
Dirk MG Seynhaeve
authored
[NFC] Productize clang-offload-extract: clean up code for command line parsing and help (#9594)
* Impose the mandatory LLVM style for clang-format * Remove any code that was trying to enhance the LLVM builtin help functionality: the extra code only made for confusing help and error messages. * Don't provide any required options, but provide reasonable defaults. * Clean up the descriptions for the help. Use easier-to-maintain heredocs for the multiline descriptions. * Use the more trivial `--stem` rather than `--output`. The `--output` option is still supported, but labeled deprecated. * Enforce double-dash long options. * Provide more context in error diagnostics. * Streamline the searches and predicates. * Modernize LLVM (e.g. remove predicated makeArrayRef). * More efficient iterators for the range-based for loops. * Extensive comments.
1 parent 8364176 commit f44d013

2 files changed

Lines changed: 178 additions & 102 deletions

File tree

‎clang/test/Driver/clang-offload-extract.c‎

Lines changed: 28 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,17 +5,22 @@
55
//
66
// RUN: clang-offload-extract --help | FileCheck %s --check-prefix CHECK-HELP
77

8-
// CHECK-HELP: OVERVIEW: A tool for extracting target images from the linked fat offload binary.
8+
// CHECK-HELP: OVERVIEW:
9+
// CHECK-HELP: A utility to extract all the target images from a
10+
// CHECK-HELP: linked fat binary, and store them in separate files.
911
// CHECK-HELP: USAGE: clang-offload-extract [options] <input file>
1012
// CHECK-HELP: OPTIONS:
1113
// CHECK-HELP: Generic Options:
12-
// CHECK-HELP: --help - Display available options (--help-hidden for more)
13-
// CHECK-HELP: --help-list - Display list of available options (--help-list-hidden for more)
14-
// CHECK-HELP: --version - Display the version of this program
15-
// CHECK-HELP: clang-offload-extract options:
16-
// CHECK-HELP: --output=<string> - Specifies prefix for the output file(s). Output file name
17-
// CHECK-HELP: is composed from this prefix and the sequential number
18-
// CHECK-HELP: of extracted image appended to the prefix.
14+
// CHECK-HELP: --help - Display available options (--help-hidden for more)
15+
// CHECK-HELP: --help-list - Display list of available options (--help-list-hidden for more)
16+
// CHECK-HELP: --version - Display the version of this program
17+
// CHECK-HELP: Utility-specific options:
18+
// CHECK-HELP: --stem=<string> - Specifies the stem for the output file(s).
19+
// CHECK-HELP: The default stem when not specified is "target.bin".
20+
// CHECK-HELP: The Output file name is composed from this stem and
21+
// CHECK-HELP: the sequential number of each extracted image appended
22+
// CHECK-HELP: to the stem:
23+
// CHECK-HELP: <stem>.<index>
1924

2025
//
2126
// Create fat offload binary with two embedded target images.
@@ -26,9 +31,22 @@
2631
// RUN: %clang -fdeclspec %s %t.wrapped.bc -o %t.fat.bin
2732

2833
//
29-
// Extract target images.
34+
// Extract target images (deprecated use model)
3035
//
31-
// RUN: clang-offload-extract --output=%t.extracted %t.fat.bin | FileCheck %s --check-prefix CHECK-EXTRACT
36+
// RUN: clang-offload-extract --output=%t.deprecated %t.fat.bin | FileCheck %s --check-prefix CHECK-DEPRECATE
37+
// CHECK-DEPRECATE: Saving target image to
38+
// CHECK-DEPRECATE: Saving target image to
39+
40+
//
41+
// Check that extracted contents match the original images.
42+
//
43+
// RUN: diff %t.deprecated.0 %t.bin0
44+
// RUN: diff %t.deprecated.1 %t.bin1
45+
46+
//
47+
// Extract target images (new use model)
48+
//
49+
// RUN: clang-offload-extract --stem=%t.extracted %t.fat.bin | FileCheck %s --check-prefix CHECK-EXTRACT
3250
// CHECK-EXTRACT: Saving target image to
3351
// CHECK-EXTRACT: Saving target image to
3452

@@ -37,7 +55,6 @@
3755
//
3856
// RUN: diff %t.extracted.0 %t.bin0
3957
// RUN: diff %t.extracted.1 %t.bin1
40-
4158
//
4259
// Some code so that we can build an offload executable from this file.
4360
//

‎clang/tools/clang-offload-extract/ClangOffloadExtract.cpp‎

Lines changed: 150 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,13 @@
55
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
66
//
77
//===----------------------------------------------------------------------===//
8-
///
9-
/// \file
10-
/// Implementation of the clang-offload-extract tool which allows extracting
11-
/// target images from linked fat offload binaries. For locating target images
12-
/// in the binary it uses information from the .tgtimg section which is added to
13-
/// the image by the clang-offload-wrapper tool. This section contains <address,
14-
/// size> pairs for all embedded target images.
15-
///
8+
//
9+
// Implementation of the clang-offload-extract tool which allows extracting
10+
// target images from linked fat offload binaries. For locating target images
11+
// in the binary it uses information from the .tgtimg section which is added to
12+
// the image by the clang-offload-wrapper tool. This section contains <address,
13+
// size> pairs for all embedded target images.
14+
//
1615
//===----------------------------------------------------------------------===//
1716

1817
#include "clang/Basic/Version.h"
@@ -22,6 +21,8 @@
2221
#include "llvm/Object/ObjectFile.h"
2322
#include "llvm/Support/CommandLine.h"
2423
#include "llvm/Support/Errc.h"
24+
#include "llvm/Support/FormatAdapters.h"
25+
#include "llvm/Support/FormatVariadic.h"
2526
#include "llvm/Support/Signals.h"
2627
#include "llvm/Support/WithColor.h"
2728
#include "llvm/Support/raw_ostream.h"
@@ -31,153 +32,211 @@
3132
using namespace llvm;
3233
using namespace llvm::object;
3334

34-
static cl::opt<bool> Help("h", cl::desc("Alias for -help"), cl::Hidden);
35-
36-
// Mark all our options with this category, everything else (except for -version
37-
// and -help) will be hidden.
35+
// Create a category to label utility-specific options; This will allow
36+
// us to distinguish those specific options from generic options and
37+
// irrelevant options
3838
static cl::OptionCategory
39-
ClangOffloadExtractCategory("clang-offload-extract options");
39+
ClangOffloadExtractCategory("Utility-specific options");
4040

41-
static cl::opt<std::string> OutputPrefix(
42-
"output", cl::Required,
43-
cl::desc("Specifies prefix for the output file(s). Output file name\n"
44-
"is composed from this prefix and the sequential number\n"
45-
"of extracted image appended to the prefix."),
46-
cl::cat(ClangOffloadExtractCategory));
47-
48-
static cl::opt<std::string> Input(cl::Positional, cl::Required,
41+
// Create options for the input and output files, each with an
42+
// appropriate default when not specified
43+
static cl::opt<std::string> Input(cl::Positional, cl::init("a.out"),
4944
cl::desc("<input file>"),
5045
cl::cat(ClangOffloadExtractCategory));
5146

52-
/// Path to the current binary.
47+
static cl::opt<std::string>
48+
FileNameStem("stem", cl::init("target.bin"),
49+
cl::desc(
50+
R"(Specifies the stem for the output file(s).
51+
The default stem when not specified is "target.bin".
52+
The Output file name is composed from this stem and
53+
the sequential number of each extracted image appended
54+
to the stem:
55+
<stem>.<index>
56+
)"),
57+
cl::cat(ClangOffloadExtractCategory));
58+
59+
// Create an alias for the deprecated option, so legacy use still works
60+
static cl::alias FileNameStemAlias(
61+
"output", cl::desc("Deprecated option, replaced by option '--stem'"),
62+
cl::aliasopt(FileNameStem), cl::cat(ClangOffloadExtractCategory));
63+
64+
// Path to the current binary
5365
static std::string ToolPath;
5466

55-
static void reportError(Error E) {
56-
logAllUnhandledErrors(std::move(E), WithColor::error(errs(), ToolPath));
67+
// Report error (and handle any deferred errors)
68+
static void reportError(Error E, Twine message = "\n") {
69+
std::string S;
70+
raw_string_ostream OSS(S);
71+
logAllUnhandledErrors(std::move(E), OSS);
72+
73+
errs() << raw_ostream::RED //
74+
<< formatv("{0,-10}", "error") //
75+
<< raw_ostream::RESET //
76+
<< S //
77+
<< formatv("{0}", fmt_pad(message, 10, 0)); //
78+
exit(1);
5779
}
5880

5981
int main(int argc, const char **argv) {
6082
sys::PrintStackTraceOnErrorSignal(argv[0]);
6183
ToolPath = argv[0];
6284

85+
// Hide non-generic options that are not in this utility's explicit
86+
// category;
87+
// Some include files bring in options that are not relevant for the
88+
// public interface of this utility (e.g. color handling or Intermediate
89+
// Representation objects)
6390
cl::HideUnrelatedOptions(ClangOffloadExtractCategory);
6491
cl::SetVersionPrinter([](raw_ostream &OS) {
65-
OS << clang::getClangToolFullVersion("clang-offload-extract") << '\n';
92+
OS << clang::getClangToolFullVersion("clang-offload-extract") << "\n";
6693
});
6794
cl::ParseCommandLineOptions(argc, argv,
68-
"A tool for extracting target images from the "
69-
"linked fat offload binary.");
70-
71-
if (Help) {
72-
cl::PrintHelpMessage();
73-
return 0;
74-
}
75-
76-
// Read input file. It should have one of the supported object file formats.
95+
R"(
96+
97+
A utility to extract all the target images from a
98+
linked fat binary, and store them in separate files.
99+
)",
100+
nullptr, nullptr, true);
101+
102+
// Read input file. It should have one of the supported object file
103+
// formats:
104+
// * Common Object File Format (COFF) : https://wiki.osdev.org/COFF
105+
// * Executable Linker Format (ELF) : https://wiki.osdev.org/ELF
106+
// This utility works on a hierarchy of objects:
107+
// = OwningBinary<ObjectFile> ObjectOrError
108+
// |_->getBinary() ObjectFile *Binary
109+
// |_->getBytesInAddress() uint8_t -
110+
// |_->section_end() section_iterator -
111+
// |_->sections() section_iterator_range -
112+
// |_: SectionRef Section
113+
// |_.getName() StringRef -
114+
// |_.getContents() StringRef -
115+
// |_.isData() bool -
116+
// |_.getAddress() uint64_t -
117+
// |_.getSize() uint64_t -
77118
Expected<OwningBinary<ObjectFile>> ObjectOrErr =
78119
ObjectFile::createObjectFile(Input);
79-
if (!ObjectOrErr) {
80-
reportError(ObjectOrErr.takeError());
81-
return 1;
120+
if (auto E = ObjectOrErr.takeError()) {
121+
reportError(std::move(E), "Input File: '" + Input + "'\n");
82122
}
83123

124+
// LLVM::ObjectFile has no constructor, but we can extract it from the
125+
// LLVM::OwningBinary object
84126
ObjectFile *Binary = ObjectOrErr->getBinary();
85127

86-
// Do we plan to support 32-bit offload binaries?
87-
if (!(isa<ELF64LEObjectFile>(Binary) || isa<COFFObjectFile>(Binary)) ||
88-
Binary->getBytesInAddress() != sizeof(void *)) {
128+
// Bitness : sizeof(void *)
129+
// 32-bit systems: 4
130+
// 64-bit systems: 8
131+
if (!(isa<ELF64LEObjectFile>(Binary) || isa<COFFObjectFile>(Binary)) //
132+
|| Binary->getBytesInAddress() != sizeof(void *) //
133+
) {
89134
reportError(
90135
createStringError(errc::invalid_argument,
91-
"only 64-bit ELF or COFF inputs are supported"));
92-
return 1;
136+
"Only 64-bit ELF or COFF inputs are supported"),
137+
"Input File: '" + Input + "'");
93138
}
94139

140+
// We are dealing with an appropriate fat binary;
141+
// Locate the section IMAGE_INFO_SECTION_NAME (which contains the
142+
// metadata on the embedded binaries)
95143
unsigned FileNum = 0;
96144

97-
for (SectionRef Section : Binary->sections()) {
98-
// Look for the .tgtimg section in the binary.
145+
for (const auto &Section : Binary->sections()) {
99146
Expected<StringRef> NameOrErr = Section.getName();
100-
if (!NameOrErr) {
101-
reportError(NameOrErr.takeError());
102-
return 1;
147+
if (auto E = NameOrErr.takeError()) {
148+
reportError(std::move(E), "Input File: '" + Input + "'\n");
103149
}
104150
if (*NameOrErr != IMAGE_INFO_SECTION_NAME)
105151
continue;
106152

107-
// This is the section we are looking for.
153+
// This is the section we are looking for;
154+
// Extract the meta information:
155+
// The IMAGE_INFO_SECTION_NAME section contains packed <address,
156+
// size> pairs describing target images that are stored in the fat
157+
// binary.
108158
Expected<StringRef> DataOrErr = Section.getContents();
109-
if (!DataOrErr) {
110-
reportError(DataOrErr.takeError());
111-
return 1;
159+
if (auto E = DataOrErr.takeError()) {
160+
reportError(std::move(E), "Input File: '" + Input + "'\n");
112161
}
113-
114-
// This section contains concatenated <address, size> pairs describing
115-
// target images that are stored in the binary. Loop over these descriptors
116-
// and extract each target image.
117-
struct ImgInfoTy {
162+
// Data type to store the metadata for an individual target image
163+
struct ImgInfoType {
118164
uintptr_t Addr;
119165
uintptr_t Size;
120166
};
121167

122-
auto ImgInfo = makeArrayRef<ImgInfoTy>(
123-
reinterpret_cast<const ImgInfoTy *>(DataOrErr->data()),
124-
DataOrErr->size() / sizeof(ImgInfoTy));
168+
// Store the metadata for all target images in an array of target
169+
// image information descriptors
170+
auto ImgInfo =
171+
ArrayRef(reinterpret_cast<const ImgInfoType *>(DataOrErr->data()),
172+
DataOrErr->size() / sizeof(ImgInfoType));
125173

126-
for (auto &Img : ImgInfo) {
174+
// Loop over the image information descriptors to extract each
175+
// target image.
176+
for (const auto &Img : ImgInfo) {
127177
// Ignore zero padding that can be inserted by the linker.
128178
if (!Img.Addr)
129179
continue;
130180

131181
// Find section which contains this image.
132-
// TODO: can use more efficient algorithm than linear search. For example
133-
// sections and images could be sorted by address then one pass performed
134-
// through both at the same time.
135-
auto ImgSec = find_if(Binary->sections(), [&Img](SectionRef Sec) {
136-
if (!Sec.isData())
137-
return false;
138-
if (Img.Addr < Sec.getAddress() ||
139-
Img.Addr + Img.Size > Sec.getAddress() + Sec.getSize())
140-
return false;
141-
return true;
142-
});
143-
if (ImgSec == Binary->section_end()) {
144-
reportError(createStringError(
145-
inconvertibleErrorCode(),
146-
"cannot find section containing <0x%lx, 0x%lx> target image",
147-
Img.Addr, Img.Size));
148-
return 1;
182+
// TODO: can use more efficient algorithm than linear search. For
183+
// example sections and images could be sorted by address then one pass
184+
// performed through both at the same time.
185+
// std::find_if
186+
// * searches for a true predicate in [first,last] =~ [first,end)
187+
// * returns end if no predicate is true
188+
// It is probably faster to track success through a bool (ImgFound)
189+
bool ImgFound = false;
190+
auto ImgSec =
191+
find_if(Binary->sections(), [&Img, &ImgFound](SectionRef Sec) {
192+
bool pred = ( //
193+
Sec.isData() //
194+
&& (Img.Addr == Sec.getAddress()) //
195+
&& (Img.Size == Sec.getSize()) //
196+
);
197+
ImgFound = ImgFound || pred;
198+
return pred;
199+
});
200+
if (!ImgFound) {
201+
202+
reportError(
203+
createStringError(
204+
inconvertibleErrorCode(),
205+
"cannot find section containing <0x%lx, 0x%lx> target image",
206+
Img.Addr, Img.Size),
207+
"Input File: '" + Input + "'\n");
149208
}
150209

151210
Expected<StringRef> SecDataOrErr = ImgSec->getContents();
152-
if (!SecDataOrErr) {
153-
reportError(SecDataOrErr.takeError());
154-
return 1;
211+
if (auto E = SecDataOrErr.takeError()) {
212+
reportError(std::move(E), "Input File: '" + Input + "'\n");
155213
}
156214

157-
// Output file name is composed from the name prefix provided by the user
158-
// and the image number which is appended to the prefix.
159-
std::string FileName = OutputPrefix + "." + std::to_string(FileNum++);
215+
// Output file name is composed from the name prefix provided by the
216+
// user and the image number which is appended to the prefix
217+
std::string FileName = FileNameStem + "." + std::to_string(FileNum++);
160218

161219
// Tell user that we are saving an image.
162-
outs() << "Saving target image to \"" << FileName << "\"\n";
220+
outs() << "Saving target image to '" << FileName << "'\n";
163221

164-
// And write image data to the output.
222+
// Write image data to the output
165223
std::error_code EC;
166224
raw_fd_ostream OS(FileName, EC);
167225
if (EC) {
168-
reportError(createFileError(FileName, EC));
169-
return 1;
226+
reportError(createFileError(FileName, EC),
227+
"Specify a different Output File ('--stem' option)\n");
170228
}
171229

172230
OS << SecDataOrErr->substr(Img.Addr - ImgSec->getAddress(), Img.Size);
173231
if (OS.has_error()) {
174-
reportError(createFileError(FileName, OS.error()));
175-
return 1;
232+
reportError(createFileError(FileName, OS.error()),
233+
"Try a different Output File ('--stem' option)");
176234
}
177-
}
235+
} // &Img: ImgInfo
178236

179-
// Binary is not expected to have more than one .tgtimg section.
237+
// Fat binary is not expected to have more than one .tgtimg section.
180238
break;
181239
}
240+
182241
return 0;
183242
}

0 commit comments

Comments
 (0)