[PATCH] D92954: [clang-offload-bundler] Add option -list
This revision was automatically updated to reflect the committed changes.
Closed by commit rG90bf3ecef4bb: [clang-offload-bundler] Add option -list
(authored by yaxunl).
Herald added a project: clang.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92954/new/
https://reviews.llvm.org/D92954
Files:
clang/test/Driver/clang-offload-bundler.c
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -63,11 +63,11 @@
cl::desc("[,...]"),
cl::cat(ClangOffloadBundlerCategory));
static cl::list
-OutputFileNames("outputs", cl::CommaSeparated, cl::OneOrMore,
+OutputFileNames("outputs", cl::CommaSeparated,
cl::desc("[,...]"),
cl::cat(ClangOffloadBundlerCategory));
static cl::list
-TargetNames("targets", cl::CommaSeparated, cl::OneOrMore,
+TargetNames("targets", cl::CommaSeparated,
cl::desc("[-,...]"),
cl::cat(ClangOffloadBundlerCategory));
static cl::opt
@@ -90,6 +90,10 @@
cl::desc("Unbundle bundled file into several output files.\n"),
cl::init(false), cl::cat(ClangOffloadBundlerCategory));
+static cl::opt
+ListBundleIDs("list", cl::desc("List bundle IDs in the bundled file.\n"),
+ cl::init(false), cl::cat(ClangOffloadBundlerCategory));
+
static cl::opt PrintExternalCommands(
"###",
cl::desc("Print any external commands that are to be executed "
@@ -134,6 +138,10 @@
/// Generic file handler interface.
class FileHandler {
public:
+ struct BundleInfo {
+StringRef BundleID;
+ };
+
FileHandler() {}
virtual ~FileHandler() {}
@@ -170,6 +178,48 @@
/// Write the bundle from \a Input into \a OS.
virtual Error WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+
+ /// List bundle IDs in \a Input.
+ virtual Error listBundleIDs(MemoryBuffer ) {
+if (Error Err = ReadHeader(Input))
+ return Err;
+
+return forEachBundle(Input, [&](const BundleInfo ) -> Error {
+ llvm::outs() << Info.BundleID << '\n';
+ Error Err = listBundleIDsCallback(Input, Info);
+ if (Err)
+return Err;
+ return Error::success();
+});
+ }
+
+ /// For each bundle in \a Input, do \a Func.
+ Error forEachBundle(MemoryBuffer ,
+ std::function Func) {
+while (true) {
+ Expected> CurTripleOrErr = ReadBundleStart(Input);
+ if (!CurTripleOrErr)
+return CurTripleOrErr.takeError();
+
+ // No more bundles.
+ if (!*CurTripleOrErr)
+break;
+
+ StringRef CurTriple = **CurTripleOrErr;
+ assert(!CurTriple.empty());
+
+ BundleInfo Info{CurTriple};
+ if (Error Err = Func(Info))
+return Err;
+}
+return Error::success();
+ }
+
+protected:
+ virtual Error listBundleIDsCallback(MemoryBuffer ,
+ const BundleInfo ) {
+return Error::success();
+ }
};
/// Handler for binary files. The bundled file will have the following format
@@ -219,22 +269,23 @@
class BinaryFileHandler final : public FileHandler {
/// Information about the bundles extracted from the header.
- struct BundleInfo final {
+ struct BinaryBundleInfo final : public BundleInfo {
/// Size of the bundle.
uint64_t Size = 0u;
/// Offset at which the bundle starts in the bundled file.
uint64_t Offset = 0u;
-BundleInfo() {}
-BundleInfo(uint64_t Size, uint64_t Offset) : Size(Size), Offset(Offset) {}
+BinaryBundleInfo() {}
+BinaryBundleInfo(uint64_t Size, uint64_t Offset)
+: Size(Size), Offset(Offset) {}
};
/// Map between a triple and the corresponding bundle information.
- StringMap BundlesInfo;
+ StringMap BundlesInfo;
/// Iterator for the bundle information that is being read.
- StringMap::iterator CurBundleInfo;
- StringMap::iterator NextBundleInfo;
+ StringMap::iterator CurBundleInfo;
+ StringMap::iterator NextBundleInfo;
/// Current bundle target to be written.
std::string CurWriteBundleTarget;
@@ -304,7 +355,7 @@
assert(BundlesInfo.find(Triple) == BundlesInfo.end() &&
"Triple is duplicated??");
- BundlesInfo[Triple] = BundleInfo(Size, Offset);
+ BundlesInfo[Triple] = BinaryBundleInfo(Size, Offset);
}
// Set the iterator to where we will start to read.
CurBundleInfo = BundlesInfo.end();
@@ -358,7 +409,7 @@
Write8byteIntegerToBuffer(OS, HeaderSize);
// Size of the bundle (adds to the next bundle's offset)
Write8byteIntegerToBuffer(OS, MB.getBufferSize());
- BundlesInfo[T] = BundleInfo(MB.getBufferSize(), HeaderSize);
+ BundlesInfo[T] =
[PATCH] D92954: [clang-offload-bundler] Add option -list
yaxunl updated this revision to Diff 314957.
yaxunl marked 4 inline comments as done.
yaxunl added a comment.
revised by Artem's comments
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92954/new/
https://reviews.llvm.org/D92954
Files:
clang/test/Driver/clang-offload-bundler.c
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -63,11 +63,11 @@
cl::desc("[,...]"),
cl::cat(ClangOffloadBundlerCategory));
static cl::list
-OutputFileNames("outputs", cl::CommaSeparated, cl::OneOrMore,
+OutputFileNames("outputs", cl::CommaSeparated,
cl::desc("[,...]"),
cl::cat(ClangOffloadBundlerCategory));
static cl::list
-TargetNames("targets", cl::CommaSeparated, cl::OneOrMore,
+TargetNames("targets", cl::CommaSeparated,
cl::desc("[-,...]"),
cl::cat(ClangOffloadBundlerCategory));
static cl::opt
@@ -90,6 +90,10 @@
cl::desc("Unbundle bundled file into several output files.\n"),
cl::init(false), cl::cat(ClangOffloadBundlerCategory));
+static cl::opt
+ListBundleIDs("list", cl::desc("List bundle IDs in the bundled file.\n"),
+ cl::init(false), cl::cat(ClangOffloadBundlerCategory));
+
static cl::opt PrintExternalCommands(
"###",
cl::desc("Print any external commands that are to be executed "
@@ -134,6 +138,10 @@
/// Generic file handler interface.
class FileHandler {
public:
+ struct BundleInfo {
+StringRef BundleID;
+ };
+
FileHandler() {}
virtual ~FileHandler() {}
@@ -170,6 +178,48 @@
/// Write the bundle from \a Input into \a OS.
virtual Error WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+
+ /// List bundle IDs in \a Input.
+ virtual Error listBundleIDs(MemoryBuffer ) {
+if (Error Err = ReadHeader(Input))
+ return Err;
+
+return forEachBundle(Input, [&](const BundleInfo ) -> Error {
+ llvm::outs() << Info.BundleID << '\n';
+ Error Err = listBundleIDsCallback(Input, Info);
+ if (Err)
+return Err;
+ return Error::success();
+});
+ }
+
+ /// For each bundle in \a Input, do \a Func.
+ Error forEachBundle(MemoryBuffer ,
+ std::function Func) {
+while (true) {
+ Expected> CurTripleOrErr = ReadBundleStart(Input);
+ if (!CurTripleOrErr)
+return CurTripleOrErr.takeError();
+
+ // No more bundles.
+ if (!*CurTripleOrErr)
+break;
+
+ StringRef CurTriple = **CurTripleOrErr;
+ assert(!CurTriple.empty());
+
+ BundleInfo Info{CurTriple};
+ if (Error Err = Func(Info))
+return Err;
+}
+return Error::success();
+ }
+
+protected:
+ virtual Error listBundleIDsCallback(MemoryBuffer ,
+ const BundleInfo ) {
+return Error::success();
+ }
};
/// Handler for binary files. The bundled file will have the following format
@@ -219,22 +269,23 @@
class BinaryFileHandler final : public FileHandler {
/// Information about the bundles extracted from the header.
- struct BundleInfo final {
+ struct BinaryBundleInfo final : public BundleInfo {
/// Size of the bundle.
uint64_t Size = 0u;
/// Offset at which the bundle starts in the bundled file.
uint64_t Offset = 0u;
-BundleInfo() {}
-BundleInfo(uint64_t Size, uint64_t Offset) : Size(Size), Offset(Offset) {}
+BinaryBundleInfo() {}
+BinaryBundleInfo(uint64_t Size, uint64_t Offset)
+: Size(Size), Offset(Offset) {}
};
/// Map between a triple and the corresponding bundle information.
- StringMap BundlesInfo;
+ StringMap BundlesInfo;
/// Iterator for the bundle information that is being read.
- StringMap::iterator CurBundleInfo;
- StringMap::iterator NextBundleInfo;
+ StringMap::iterator CurBundleInfo;
+ StringMap::iterator NextBundleInfo;
/// Current bundle target to be written.
std::string CurWriteBundleTarget;
@@ -304,7 +355,7 @@
assert(BundlesInfo.find(Triple) == BundlesInfo.end() &&
"Triple is duplicated??");
- BundlesInfo[Triple] = BundleInfo(Size, Offset);
+ BundlesInfo[Triple] = BinaryBundleInfo(Size, Offset);
}
// Set the iterator to where we will start to read.
CurBundleInfo = BundlesInfo.end();
@@ -358,7 +409,7 @@
Write8byteIntegerToBuffer(OS, HeaderSize);
// Size of the bundle (adds to the next bundle's offset)
Write8byteIntegerToBuffer(OS, MB.getBufferSize());
- BundlesInfo[T] = BundleInfo(MB.getBufferSize(), HeaderSize);
+ BundlesInfo[T] = BinaryBundleInfo(MB.getBufferSize(), HeaderSize);
HeaderSize += MB.getBufferSize();
// Size of the triple
[PATCH] D92954: [clang-offload-bundler] Add option -list
yaxunl marked 4 inline comments as done.
yaxunl added inline comments.
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:176
+ /// List bundle IDs in \a Input.
+ virtual Error listBundleIDs(MemoryBuffer ) {
+if (Error Err = ReadHeader(Input))
tra wrote:
> Now that listBundleIDs is only used by ` ListBundleIDsInFile()`, perhaps it
> could all be simplified and moved out of the class.
listBundleIDsCallback needs to be a virtual function and it is called by
listBundleIDs. Keep listBundleIDs as a member function together with
listBundleIDsCallback shows their relation and is more readable.
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:882
+// List bundle IDs. Return true if an error was found.
+static Error ListBundleIDsInFile() {
+ // Open Input file.
tra wrote:
> I'd pass InputFileNames as an argument. Makes it easier to tell what the
> function needs.
done
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:1031-1035
+ Error = true;
+ reportError(createStringError(
+ errc::invalid_argument,
+ "for the --outputs option: must be specified at least once!"));
+}
tra wrote:
> Does it make sense to continue once we know that CLI options are wrong?
> If we just early-exit with an error that may help simplifying the code below
> a bit.
>
>
done
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:1047
+errc::invalid_argument, "-unbundle and -list cannot be used
together"));
+ } else if (ListBundleIDs) {
+if (InputFileNames.size() != 1) {
tra wrote:
> Perhaps we can separate list option processing from bundle/unbundle?
>
> I think if we could do something like this it would be more readable:
> ```
> if (ListBundleIDs) {
> if (Unbundle) {
> error...
> exit.
> }
> ... other list-specific checks...
> ListBundleIDsInFile(InputFileNames)
> exit 0;
> }
>
> // complicated bundle/unbundle logic can proceed without having to bother
> about `list` option.
> ```
done
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92954/new/
https://reviews.llvm.org/D92954
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92954: [clang-offload-bundler] Add option -list
tra added inline comments.
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:176
+ /// List bundle IDs in \a Input.
+ virtual Error listBundleIDs(MemoryBuffer ) {
+if (Error Err = ReadHeader(Input))
Now that listBundleIDs is only used by ` ListBundleIDsInFile()`, perhaps it
could all be simplified and moved out of the class.
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:882
+// List bundle IDs. Return true if an error was found.
+static Error ListBundleIDsInFile() {
+ // Open Input file.
I'd pass InputFileNames as an argument. Makes it easier to tell what the
function needs.
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:1031-1035
+ Error = true;
+ reportError(createStringError(
+ errc::invalid_argument,
+ "for the --outputs option: must be specified at least once!"));
+}
Does it make sense to continue once we know that CLI options are wrong?
If we just early-exit with an error that may help simplifying the code below a
bit.
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:1047
+errc::invalid_argument, "-unbundle and -list cannot be used
together"));
+ } else if (ListBundleIDs) {
+if (InputFileNames.size() != 1) {
Perhaps we can separate list option processing from bundle/unbundle?
I think if we could do something like this it would be more readable:
```
if (ListBundleIDs) {
if (Unbundle) {
error...
exit.
}
... other list-specific checks...
ListBundleIDsInFile(InputFileNames)
exit 0;
}
// complicated bundle/unbundle logic can proceed without having to bother about
`list` option.
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92954/new/
https://reviews.llvm.org/D92954
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92954: [clang-offload-bundler] Add option -list
yaxunl added a comment. ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92954/new/ https://reviews.llvm.org/D92954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92954: [clang-offload-bundler] Add option -list
yaxunl added a comment. ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92954/new/ https://reviews.llvm.org/D92954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92954: [clang-offload-bundler] Add option -list
yaxunl updated this revision to Diff 311187.
yaxunl marked 3 inline comments as done.
yaxunl added a comment.
Revised by Artem's comments.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92954/new/
https://reviews.llvm.org/D92954
Files:
clang/test/Driver/clang-offload-bundler.c
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -62,11 +62,11 @@
cl::desc("[,...]"),
cl::cat(ClangOffloadBundlerCategory));
static cl::list
-OutputFileNames("outputs", cl::CommaSeparated, cl::OneOrMore,
+OutputFileNames("outputs", cl::CommaSeparated,
cl::desc("[,...]"),
cl::cat(ClangOffloadBundlerCategory));
static cl::list
-TargetNames("targets", cl::CommaSeparated, cl::OneOrMore,
+TargetNames("targets", cl::CommaSeparated,
cl::desc("[-,...]"),
cl::cat(ClangOffloadBundlerCategory));
static cl::opt
@@ -89,6 +89,10 @@
cl::desc("Unbundle bundled file into several output files.\n"),
cl::init(false), cl::cat(ClangOffloadBundlerCategory));
+static cl::opt
+ListBundleIDs("list", cl::desc("List bundle IDs in the bundled file.\n"),
+ cl::init(false), cl::cat(ClangOffloadBundlerCategory));
+
static cl::opt PrintExternalCommands(
"###",
cl::desc("Print any external commands that are to be executed "
@@ -127,6 +131,10 @@
/// Generic file handler interface.
class FileHandler {
public:
+ struct BundleInfo {
+StringRef BundleID;
+ };
+
FileHandler() {}
virtual ~FileHandler() {}
@@ -163,6 +171,48 @@
/// Write the bundle from \a Input into \a OS.
virtual Error WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+
+ /// List bundle IDs in \a Input.
+ virtual Error listBundleIDs(MemoryBuffer ) {
+if (Error Err = ReadHeader(Input))
+ return Err;
+
+return forEachBundle(Input, [&](const BundleInfo ) -> Error {
+ llvm::outs() << Info.BundleID << '\n';
+ Error Err = listBundleIDsCallback(Input, Info);
+ if (Err)
+return Err;
+ return Error::success();
+});
+ }
+
+ /// For each bundle in \a Input, do \a Func.
+ Error forEachBundle(MemoryBuffer ,
+ std::function Func) {
+while (true) {
+ Expected> CurTripleOrErr = ReadBundleStart(Input);
+ if (!CurTripleOrErr)
+return CurTripleOrErr.takeError();
+
+ // No more bundles.
+ if (!*CurTripleOrErr)
+break;
+
+ StringRef CurTriple = **CurTripleOrErr;
+ assert(!CurTriple.empty());
+
+ BundleInfo Info{CurTriple};
+ if (Error Err = Func(Info))
+return Err;
+}
+return Error::success();
+ }
+
+protected:
+ virtual Error listBundleIDsCallback(MemoryBuffer ,
+ const BundleInfo ) {
+return Error::success();
+ }
};
/// Handler for binary files. The bundled file will have the following format
@@ -212,22 +262,23 @@
class BinaryFileHandler final : public FileHandler {
/// Information about the bundles extracted from the header.
- struct BundleInfo final {
+ struct BinaryBundleInfo final : public BundleInfo {
/// Size of the bundle.
uint64_t Size = 0u;
/// Offset at which the bundle starts in the bundled file.
uint64_t Offset = 0u;
-BundleInfo() {}
-BundleInfo(uint64_t Size, uint64_t Offset) : Size(Size), Offset(Offset) {}
+BinaryBundleInfo() {}
+BinaryBundleInfo(uint64_t Size, uint64_t Offset)
+: Size(Size), Offset(Offset) {}
};
/// Map between a triple and the corresponding bundle information.
- StringMap BundlesInfo;
+ StringMap BundlesInfo;
/// Iterator for the bundle information that is being read.
- StringMap::iterator CurBundleInfo;
- StringMap::iterator NextBundleInfo;
+ StringMap::iterator CurBundleInfo;
+ StringMap::iterator NextBundleInfo;
/// Current bundle target to be written.
std::string CurWriteBundleTarget;
@@ -297,7 +348,7 @@
assert(BundlesInfo.find(Triple) == BundlesInfo.end() &&
"Triple is duplicated??");
- BundlesInfo[Triple] = BundleInfo(Size, Offset);
+ BundlesInfo[Triple] = BinaryBundleInfo(Size, Offset);
}
// Set the iterator to where we will start to read.
CurBundleInfo = BundlesInfo.end();
@@ -351,7 +402,7 @@
Write8byteIntegerToBuffer(OS, HeaderSize);
// Size of the bundle (adds to the next bundle's offset)
Write8byteIntegerToBuffer(OS, MB.getBufferSize());
- BundlesInfo[T] = BundleInfo(MB.getBufferSize(), HeaderSize);
+ BundlesInfo[T] = BinaryBundleInfo(MB.getBufferSize(), HeaderSize);
HeaderSize += MB.getBufferSize();
// Size of the triple
[PATCH] D92954: [clang-offload-bundler] Add option -list
yaxunl marked 3 inline comments as done.
yaxunl added inline comments.
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:188
+
+ if (Error Err = Func())
+return Err;
tra wrote:
> Now, if we could save the triple in a `BundleInfo` when it's parsed, and pass
> `BundleInfo` to `Func()` that would make the iterator more useful.
>
done
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:320-321
ReadChars += TripleSize;
+ if (ListBundleIDs)
+llvm::outs() << Triple << '\n';
tra wrote:
> I'm still not quite happy with the fact that the listing is interleaving with
> reading the bundle.
> I think the code would benefit from further refactoring that would separate
> bundle reading from what we do with the bundles we've read.
>
good point. this can be moved to the lambda without incurring significant
overhead.
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:641
+// start of each bundle which is done by forEachBundle.
+return forEachBundle(Input, []() { return Error::success(); });
+ }
tra wrote:
> Once BundleInfo carries the triple, `ListBundleIDs` could then be changed to
> print the bundle info and moved into the FileHandler class:
>
> ```
> if (Error Err = ReadHeader(Input))
> return Err;
>
> return forEachBundle(Input, [](BundleInfo bundle) { llvm::outs() <<
> bundle.triple << '\n'});
> ```
>
> All other printouts of the triple should no longer be necessary.
done
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92954/new/
https://reviews.llvm.org/D92954
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92954: [clang-offload-bundler] Add option -list
tra added inline comments.
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:188
+
+ if (Error Err = Func())
+return Err;
Now, if we could save the triple in a `BundleInfo` when it's parsed, and pass
`BundleInfo` to `Func()` that would make the iterator more useful.
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:320-321
ReadChars += TripleSize;
+ if (ListBundleIDs)
+llvm::outs() << Triple << '\n';
I'm still not quite happy with the fact that the listing is interleaving with
reading the bundle.
I think the code would benefit from further refactoring that would separate
bundle reading from what we do with the bundles we've read.
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:641
+// start of each bundle which is done by forEachBundle.
+return forEachBundle(Input, []() { return Error::success(); });
+ }
Once BundleInfo carries the triple, `ListBundleIDs` could then be changed to
print the bundle info and moved into the FileHandler class:
```
if (Error Err = ReadHeader(Input))
return Err;
return forEachBundle(Input, [](BundleInfo bundle) { llvm::outs() <<
bundle.triple << '\n'});
```
All other printouts of the triple should no longer be necessary.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92954/new/
https://reviews.llvm.org/D92954
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92954: [clang-offload-bundler] Add option -list
yaxunl updated this revision to Diff 310751.
yaxunl added a comment.
Remove unnecessary formatting changes.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92954/new/
https://reviews.llvm.org/D92954
Files:
clang/test/Driver/clang-offload-bundler.c
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -62,11 +62,11 @@
cl::desc("[,...]"),
cl::cat(ClangOffloadBundlerCategory));
static cl::list
-OutputFileNames("outputs", cl::CommaSeparated, cl::OneOrMore,
+OutputFileNames("outputs", cl::CommaSeparated,
cl::desc("[,...]"),
cl::cat(ClangOffloadBundlerCategory));
static cl::list
-TargetNames("targets", cl::CommaSeparated, cl::OneOrMore,
+TargetNames("targets", cl::CommaSeparated,
cl::desc("[-,...]"),
cl::cat(ClangOffloadBundlerCategory));
static cl::opt
@@ -89,6 +89,10 @@
cl::desc("Unbundle bundled file into several output files.\n"),
cl::init(false), cl::cat(ClangOffloadBundlerCategory));
+static cl::opt
+ListBundleIDs("list", cl::desc("List bundle IDs in the bundled file.\n"),
+ cl::init(false), cl::cat(ClangOffloadBundlerCategory));
+
static cl::opt PrintExternalCommands(
"###",
cl::desc("Print any external commands that are to be executed "
@@ -163,6 +167,29 @@
/// Write the bundle from \a Input into \a OS.
virtual Error WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+
+ /// List bundle IDs in \a Input.
+ virtual Error listBundleIDs(MemoryBuffer ) = 0;
+
+ /// For each bundle in \a Input, do \a Func.
+ Error forEachBundle(MemoryBuffer , std::function Func) {
+while (true) {
+ Expected> CurTripleOrErr = ReadBundleStart(Input);
+ if (!CurTripleOrErr)
+return CurTripleOrErr.takeError();
+
+ // No more bundles.
+ if (!*CurTripleOrErr)
+break;
+
+ StringRef CurTriple = **CurTripleOrErr;
+ assert(!CurTriple.empty());
+
+ if (Error Err = Func())
+return Err;
+}
+return Error::success();
+ }
};
/// Handler for binary files. The bundled file will have the following format
@@ -290,6 +317,8 @@
StringRef Triple(()[ReadChars], TripleSize);
ReadChars += TripleSize;
+ if (ListBundleIDs)
+llvm::outs() << Triple << '\n';
// Check if the offset and size make sense.
if (!Offset || Offset + Size > FC.size())
@@ -376,6 +405,11 @@
OS.write(Input.getBufferStart(), Input.getBufferSize());
return Error::success();
}
+
+ Error listBundleIDs(MemoryBuffer ) final {
+// List bundle IDs in a binary file only needs go through the header.
+return ReadHeader(Input);
+ }
};
namespace {
@@ -472,8 +506,11 @@
IsOffloadSection(*CurrentSection);
if (!TripleOrErr)
return TripleOrErr.takeError();
- if (*TripleOrErr)
+ if (*TripleOrErr) {
+if (ListBundleIDs)
+ llvm::outs() << **TripleOrErr << '\n';
return **TripleOrErr;
+ }
}
return None;
}
@@ -595,6 +632,15 @@
return Error::success();
}
+ Error listBundleIDs(MemoryBuffer ) final {
+if (Error Err = ReadHeader(Input))
+ return Err;
+
+// To list bundle IDs in bundled object files we only need to read the
+// start of each bundle which is done by forEachBundle.
+return forEachBundle(Input, []() { return Error::success(); });
+ }
+
private:
static Error executeObjcopy(StringRef Objcopy, ArrayRef Args) {
// If the user asked for the commands to be printed out, we do that
@@ -657,7 +703,10 @@
// Next time we read after the new line.
++ReadChars;
-return StringRef(()[TripleStart], TripleEnd - TripleStart);
+auto Triple = StringRef(()[TripleStart], TripleEnd - TripleStart);
+if (ListBundleIDs)
+ llvm::outs() << Triple << '\n';
+return Triple;
}
Error ReadBundleEnd(MemoryBuffer ) final {
@@ -715,6 +764,22 @@
BundleEndString =
"\n" + Comment.str() + " " OFFLOAD_BUNDLER_MAGIC_STR "__END__ ";
}
+
+ Error listBundleIDs(MemoryBuffer ) final {
+if (Error Err = ReadHeader(Input))
+ return Err;
+
+return forEachBundle(Input, [&]() -> Error {
+ // TODO: To list bundle IDs in a bundled text file we need to go through
+ // all bundles. The format of bundled text file may need to include a
+ // header if the performance of listing bundle IDs of bundled text file is
+ // important.
+ ReadChars = Input.getBuffer().find(BundleEndString, ReadChars);
+ if (Error Err = ReadBundleEnd(Input))
+return Err;
+ return Error::success();
+});
+ }
};
/// Return an
[PATCH] D92954: [clang-offload-bundler] Add option -list
yaxunl added inline comments. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:763 + +// Create an intermediate temporary file for reading the bundles. +TempFileHandlerRAII TempFiles; tra wrote: > Having to create a temporary file in order to *list* content of the bundle > strikes me as rather odd. > It looks like in order to list the content we actually do bundle unpacking, > printing bundled content in the process, and discard the results afterwards. > Is that so? > > Perhaps it would be better to refactor the code a bit and separate iteration > over the bundle from what each iteration does. > E.g. make a function `forEachBundledFile(input, lambda)` and then pass a > function that writes things out for normal operations and a function which > just prints the info in case of `--list`. It may simplify the code a bit as > right now you have to copy/paste the loops iterating over ReadBundleStart. done. thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92954/new/ https://reviews.llvm.org/D92954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92954: [clang-offload-bundler] Add option -list
yaxunl updated this revision to Diff 310748.
yaxunl marked an inline comment as done.
yaxunl added a comment.
Revised by Artem's comments: removing unnecessary output to temporary file,
extract forEachBundle.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92954/new/
https://reviews.llvm.org/D92954
Files:
clang/test/Driver/clang-offload-bundler.c
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -62,11 +62,11 @@
cl::desc("[,...]"),
cl::cat(ClangOffloadBundlerCategory));
static cl::list
-OutputFileNames("outputs", cl::CommaSeparated, cl::OneOrMore,
+OutputFileNames("outputs", cl::CommaSeparated,
cl::desc("[,...]"),
cl::cat(ClangOffloadBundlerCategory));
static cl::list
-TargetNames("targets", cl::CommaSeparated, cl::OneOrMore,
+TargetNames("targets", cl::CommaSeparated,
cl::desc("[-,...]"),
cl::cat(ClangOffloadBundlerCategory));
static cl::opt
@@ -89,6 +89,10 @@
cl::desc("Unbundle bundled file into several output files.\n"),
cl::init(false), cl::cat(ClangOffloadBundlerCategory));
+static cl::opt
+ListBundleIDs("list", cl::desc("List bundle IDs in the bundled file.\n"),
+ cl::init(false), cl::cat(ClangOffloadBundlerCategory));
+
static cl::opt PrintExternalCommands(
"###",
cl::desc("Print any external commands that are to be executed "
@@ -109,6 +113,41 @@
/// Path to the current binary.
static std::string BundlerExecutable;
+namespace {
+
+// This class implements a list of temporary files that are removed upon
+// object destruction.
+class TempFileHandlerRAII {
+public:
+ ~TempFileHandlerRAII() {
+for (const auto : Files)
+ sys::fs::remove(File);
+ }
+
+ // Creates temporary file with given contents.
+ Expected Create(Optional> Contents) {
+SmallString<128u> File;
+if (std::error_code EC =
+sys::fs::createTemporaryFile("clang-offload-bundler", "tmp", File))
+ return createFileError(File, EC);
+Files.push_front(File);
+
+if (Contents) {
+ std::error_code EC;
+ raw_fd_ostream OS(File, EC);
+ if (EC)
+return createFileError(File, EC);
+ OS.write(Contents->data(), Contents->size());
+}
+return Files.front();
+ }
+
+private:
+ std::forward_list> Files;
+};
+
+} // end anonymous namespace
+
/// Obtain the offload kind and real machine triple out of the target
/// information specified by the user.
static void getOffloadKindAndTriple(StringRef Target, StringRef ,
@@ -163,6 +202,29 @@
/// Write the bundle from \a Input into \a OS.
virtual Error WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+
+ /// List bundle IDs in \a Input.
+ virtual Error listBundleIDs(MemoryBuffer ) = 0;
+
+ /// For each bundle in \a Input, do \a Func.
+ Error forEachBundle(MemoryBuffer , std::function Func) {
+while (true) {
+ Expected> CurTripleOrErr = ReadBundleStart(Input);
+ if (!CurTripleOrErr)
+return CurTripleOrErr.takeError();
+
+ // No more bundles.
+ if (!*CurTripleOrErr)
+break;
+
+ StringRef CurTriple = **CurTripleOrErr;
+ assert(!CurTriple.empty());
+
+ if (Error Err = Func())
+return Err;
+}
+return Error::success();
+ }
};
/// Handler for binary files. The bundled file will have the following format
@@ -290,6 +352,8 @@
StringRef Triple(()[ReadChars], TripleSize);
ReadChars += TripleSize;
+ if (ListBundleIDs)
+llvm::outs() << Triple << '\n';
// Check if the offset and size make sense.
if (!Offset || Offset + Size > FC.size())
@@ -376,42 +440,13 @@
OS.write(Input.getBufferStart(), Input.getBufferSize());
return Error::success();
}
-};
-
-namespace {
-// This class implements a list of temporary files that are removed upon
-// object destruction.
-class TempFileHandlerRAII {
-public:
- ~TempFileHandlerRAII() {
-for (const auto : Files)
- sys::fs::remove(File);
+ Error listBundleIDs(MemoryBuffer ) final {
+// List bundle IDs in a binary file only needs go through the header.
+return ReadHeader(Input);
}
-
- // Creates temporary file with given contents.
- Expected Create(Optional> Contents) {
-SmallString<128u> File;
-if (std::error_code EC =
-sys::fs::createTemporaryFile("clang-offload-bundler", "tmp", File))
- return createFileError(File, EC);
-Files.push_front(File);
-
-if (Contents) {
- std::error_code EC;
- raw_fd_ostream OS(File, EC);
- if (EC)
-return createFileError(File, EC);
- OS.write(Contents->data(), Contents->size());
-
[PATCH] D92954: [clang-offload-bundler] Add option -list
tra added inline comments. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:763 + +// Create an intermediate temporary file for reading the bundles. +TempFileHandlerRAII TempFiles; Having to create a temporary file in order to *list* content of the bundle strikes me as rather odd. It looks like in order to list the content we actually do bundle unpacking, printing bundled content in the process, and discard the results afterwards. Is that so? Perhaps it would be better to refactor the code a bit and separate iteration over the bundle from what each iteration does. E.g. make a function `forEachBundledFile(input, lambda)` and then pass a function that writes things out for normal operations and a function which just prints the info in case of `--list`. It may simplify the code a bit as right now you have to copy/paste the loops iterating over ReadBundleStart. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92954/new/ https://reviews.llvm.org/D92954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D92954: [clang-offload-bundler] Add option -list
yaxunl created this revision.
yaxunl added a reviewer: tra.
yaxunl requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.
clang-offload-bundler is not only used by clang driver
to bundle/unbundle files for offloading toolchains,
but also used by out of tree tools to unbundle
fat binaries generated by clang. It is important
to be able to list the bundle IDs in a bundled
file so that the bundles can be extracted.
This patch adds an option -list to list bundle
ID's in a bundled file. Each bundle ID is separated
by new line. If the file is not a bundled file
nothing is output and returns 0.
https://reviews.llvm.org/D92954
Files:
clang/test/Driver/clang-offload-bundler.c
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
Index: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
===
--- clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
+++ clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
@@ -62,11 +62,11 @@
cl::desc("[,...]"),
cl::cat(ClangOffloadBundlerCategory));
static cl::list
-OutputFileNames("outputs", cl::CommaSeparated, cl::OneOrMore,
+OutputFileNames("outputs", cl::CommaSeparated,
cl::desc("[,...]"),
cl::cat(ClangOffloadBundlerCategory));
static cl::list
-TargetNames("targets", cl::CommaSeparated, cl::OneOrMore,
+TargetNames("targets", cl::CommaSeparated,
cl::desc("[-,...]"),
cl::cat(ClangOffloadBundlerCategory));
static cl::opt
@@ -89,6 +89,10 @@
cl::desc("Unbundle bundled file into several output files.\n"),
cl::init(false), cl::cat(ClangOffloadBundlerCategory));
+static cl::opt
+ListBundleIDs("list", cl::desc("List bundle IDs in the bundled file.\n"),
+ cl::init(false), cl::cat(ClangOffloadBundlerCategory));
+
static cl::opt PrintExternalCommands(
"###",
cl::desc("Print any external commands that are to be executed "
@@ -109,6 +113,41 @@
/// Path to the current binary.
static std::string BundlerExecutable;
+namespace {
+
+// This class implements a list of temporary files that are removed upon
+// object destruction.
+class TempFileHandlerRAII {
+public:
+ ~TempFileHandlerRAII() {
+for (const auto : Files)
+ sys::fs::remove(File);
+ }
+
+ // Creates temporary file with given contents.
+ Expected Create(Optional> Contents) {
+SmallString<128u> File;
+if (std::error_code EC =
+sys::fs::createTemporaryFile("clang-offload-bundler", "tmp", File))
+ return createFileError(File, EC);
+Files.push_front(File);
+
+if (Contents) {
+ std::error_code EC;
+ raw_fd_ostream OS(File, EC);
+ if (EC)
+return createFileError(File, EC);
+ OS.write(Contents->data(), Contents->size());
+}
+return Files.front();
+ }
+
+private:
+ std::forward_list> Files;
+};
+
+} // end anonymous namespace
+
/// Obtain the offload kind and real machine triple out of the target
/// information specified by the user.
static void getOffloadKindAndTriple(StringRef Target, StringRef ,
@@ -163,6 +202,9 @@
/// Write the bundle from \a Input into \a OS.
virtual Error WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+
+ /// List bundle IDs in \a Input.
+ virtual Error listBundleIDs(MemoryBuffer ) = 0;
};
/// Handler for binary files. The bundled file will have the following format
@@ -290,6 +332,8 @@
StringRef Triple(()[ReadChars], TripleSize);
ReadChars += TripleSize;
+ if (ListBundleIDs)
+llvm::outs() << Triple << '\n';
// Check if the offset and size make sense.
if (!Offset || Offset + Size > FC.size())
@@ -376,42 +420,13 @@
OS.write(Input.getBufferStart(), Input.getBufferSize());
return Error::success();
}
-};
-
-namespace {
-
-// This class implements a list of temporary files that are removed upon
-// object destruction.
-class TempFileHandlerRAII {
-public:
- ~TempFileHandlerRAII() {
-for (const auto : Files)
- sys::fs::remove(File);
- }
- // Creates temporary file with given contents.
- Expected Create(Optional> Contents) {
-SmallString<128u> File;
-if (std::error_code EC =
-sys::fs::createTemporaryFile("clang-offload-bundler", "tmp", File))
- return createFileError(File, EC);
-Files.push_front(File);
-
-if (Contents) {
- std::error_code EC;
- raw_fd_ostream OS(File, EC);
- if (EC)
-return createFileError(File, EC);
- OS.write(Contents->data(), Contents->size());
-}
-return Files.front();
+ Error listBundleIDs(MemoryBuffer ) final {
+// List bundle IDs in a binary file only needs go through the header.
+return ReadHeader(Input);
}
-
-private:
- std::forward_list> Files;
};
-} // end anonymous namespace
/// Handler for object
