[PATCH] D67031: [Clang][Bundler] Error reporting improvements
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdd501045cdea: [Clang][Bundler] Error reporting improvements
(authored by sdmitriev).
Changed prior to commit:
https://reviews.llvm.org/D67031?vs=224726=226521#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67031/new/
https://reviews.llvm.org/D67031
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
@@ -26,15 +26,17 @@
#include "llvm/Object/ObjectFile.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorOr.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
-#include "llvm/Support/raw_ostream.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/StringSaver.h"
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
#include
#include
#include
@@ -42,6 +44,7 @@
#include
#include
#include
+#include
using namespace llvm;
using namespace llvm::object;
@@ -123,36 +126,37 @@
virtual ~FileHandler() {}
/// Update the file handler with information from the header of the bundled
- /// file
- virtual void ReadHeader(MemoryBuffer ) = 0;
+ /// file.
+ virtual Error ReadHeader(MemoryBuffer ) = 0;
- /// Read the marker of the next bundled to be read in the file. The triple of
- /// the target associated with that bundle is returned. An empty string is
- /// returned if there are no more bundles to be read.
- virtual StringRef ReadBundleStart(MemoryBuffer ) = 0;
+ /// Read the marker of the next bundled to be read in the file. The bundle
+ /// name is returned if there is one in the file, or `None` if there are no
+ /// more bundles to be read.
+ virtual Expected>
+ ReadBundleStart(MemoryBuffer ) = 0;
/// Read the marker that closes the current bundle.
- virtual void ReadBundleEnd(MemoryBuffer ) = 0;
+ virtual Error ReadBundleEnd(MemoryBuffer ) = 0;
/// Read the current bundle and write the result into the stream \a OS.
- virtual void ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+ virtual Error ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
/// Write the header of the bundled file to \a OS based on the information
/// gathered from \a Inputs.
- virtual void WriteHeader(raw_fd_ostream ,
- ArrayRef> Inputs) = 0;
+ virtual Error WriteHeader(raw_fd_ostream ,
+ArrayRef> Inputs) = 0;
/// Write the marker that initiates a bundle for the triple \a TargetTriple to
/// \a OS.
- virtual void WriteBundleStart(raw_fd_ostream , StringRef TargetTriple) = 0;
+ virtual Error WriteBundleStart(raw_fd_ostream ,
+ StringRef TargetTriple) = 0;
/// Write the marker that closes a bundle for the triple \a TargetTriple to \a
- /// OS. Return true if any error was found.
-
- virtual bool WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
+ /// OS.
+ virtual Error WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
/// Write the bundle from \a Input into \a OS.
- virtual void WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+ virtual Error WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
};
/// Handler for binary files. The bundled file will have the following format
@@ -224,7 +228,7 @@
~BinaryFileHandler() final {}
- void ReadHeader(MemoryBuffer ) final {
+ Error ReadHeader(MemoryBuffer ) final {
StringRef FC = Input.getBuffer();
// Initialize the current bundle with the end of the container.
@@ -233,16 +237,16 @@
// Check if buffer is smaller than magic string.
size_t ReadChars = sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1;
if (ReadChars > FC.size())
- return;
+ return Error::success();
// Check if no magic was found.
StringRef Magic(FC.data(), sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1);
if (!Magic.equals(OFFLOAD_BUNDLER_MAGIC_STR))
- return;
+ return Error::success();
// Read number of bundles.
if (ReadChars + 8 > FC.size())
- return;
+ return Error::success();
uint64_t NumberOfBundles = Read8byteIntegerFromBuffer(FC, ReadChars);
ReadChars += 8;
@@ -252,35 +256,35 @@
// Read offset.
if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
uint64_t Offset = Read8byteIntegerFromBuffer(FC, ReadChars);
ReadChars += 8;
// Read size.
if (ReadChars + 8 > FC.size())
-return;
+
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
ABataev accepted this revision. ABataev added a comment. This revision is now accepted and ready to land. LG CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev added inline comments. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:133 /// returned if there are no more bundles to be read. - virtual StringRef ReadBundleStart(MemoryBuffer ) = 0; + virtual Expected> + ReadBundleStart(MemoryBuffer ) = 0; ABataev wrote: > sdmitriev wrote: > > sdmitriev wrote: > > > ABataev wrote: > > > > Do we really need an inner `Optional` here? > > > The idea was to return `StringRef` with bundle name when we have still > > > have bundles and `None` when there are no more bundles in the file (BTW > > > comment has to be updated to describe this). But I can restore the old > > > convention where empty bundle name means 'no more bundles' in the file. > > > In terms of functionality that would be the same, though use of > > > `Optional<...>` makes intentions more explicit)) > > I have updated comment to describe the intended behavior. > > @ABataev do you insist on removing inner `Optional<>` and restoring the > > current convention where empty string means the end of bundles in the file? > The problem here is that `Expected` already handles the state and we have > inner `Optional` for the same reason. Can we reuse `Expected` to indicate > that there are no more bundles? Well, `Expected` encodes two possible states (either Error or Value), but we need to encode three states - Error, Value or None. I do not have ideas how to do it without adding inner `Optional`. If you have, please share. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
ABataev added inline comments. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:133 /// returned if there are no more bundles to be read. - virtual StringRef ReadBundleStart(MemoryBuffer ) = 0; + virtual Expected> + ReadBundleStart(MemoryBuffer ) = 0; sdmitriev wrote: > sdmitriev wrote: > > ABataev wrote: > > > Do we really need an inner `Optional` here? > > The idea was to return `StringRef` with bundle name when we have still have > > bundles and `None` when there are no more bundles in the file (BTW comment > > has to be updated to describe this). But I can restore the old convention > > where empty bundle name means 'no more bundles' in the file. In terms of > > functionality that would be the same, though use of `Optional<...>` makes > > intentions more explicit)) > I have updated comment to describe the intended behavior. > @ABataev do you insist on removing inner `Optional<>` and restoring the > current convention where empty string means the end of bundles in the file? The problem here is that `Expected` already handles the state and we have inner `Optional` for the same reason. Can we reuse `Expected` to indicate that there are no more bundles? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev added inline comments. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:133 /// returned if there are no more bundles to be read. - virtual StringRef ReadBundleStart(MemoryBuffer ) = 0; + virtual Expected> + ReadBundleStart(MemoryBuffer ) = 0; sdmitriev wrote: > ABataev wrote: > > Do we really need an inner `Optional` here? > The idea was to return `StringRef` with bundle name when we have still have > bundles and `None` when there are no more bundles in the file (BTW comment > has to be updated to describe this). But I can restore the old convention > where empty bundle name means 'no more bundles' in the file. In terms of > functionality that would be the same, though use of `Optional<...>` makes > intentions more explicit)) I have updated comment to describe the intended behavior. @ABataev do you insist on removing inner `Optional<>` and restoring the current convention where empty string means the end of bundles in the file? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev updated this revision to Diff 224726.
sdmitriev added a comment.
Rebased patch.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67031/new/
https://reviews.llvm.org/D67031
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
@@ -26,15 +26,17 @@
#include "llvm/Object/ObjectFile.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorOr.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
-#include "llvm/Support/raw_ostream.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/StringSaver.h"
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
#include
#include
#include
@@ -42,6 +44,7 @@
#include
#include
#include
+#include
using namespace llvm;
using namespace llvm::object;
@@ -123,36 +126,37 @@
virtual ~FileHandler() {}
/// Update the file handler with information from the header of the bundled
- /// file
- virtual void ReadHeader(MemoryBuffer ) = 0;
+ /// file.
+ virtual Error ReadHeader(MemoryBuffer ) = 0;
- /// Read the marker of the next bundled to be read in the file. The triple of
- /// the target associated with that bundle is returned. An empty string is
- /// returned if there are no more bundles to be read.
- virtual StringRef ReadBundleStart(MemoryBuffer ) = 0;
+ /// Read the marker of the next bundled to be read in the file. The bundle
+ /// name is returned if there is one in the file, or `None` if there are no
+ /// more bundles to be read.
+ virtual Expected>
+ ReadBundleStart(MemoryBuffer ) = 0;
/// Read the marker that closes the current bundle.
- virtual void ReadBundleEnd(MemoryBuffer ) = 0;
+ virtual Error ReadBundleEnd(MemoryBuffer ) = 0;
/// Read the current bundle and write the result into the stream \a OS.
- virtual void ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+ virtual Error ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
/// Write the header of the bundled file to \a OS based on the information
/// gathered from \a Inputs.
- virtual void WriteHeader(raw_fd_ostream ,
- ArrayRef> Inputs) = 0;
+ virtual Error WriteHeader(raw_fd_ostream ,
+ArrayRef> Inputs) = 0;
/// Write the marker that initiates a bundle for the triple \a TargetTriple to
/// \a OS.
- virtual void WriteBundleStart(raw_fd_ostream , StringRef TargetTriple) = 0;
+ virtual Error WriteBundleStart(raw_fd_ostream ,
+ StringRef TargetTriple) = 0;
/// Write the marker that closes a bundle for the triple \a TargetTriple to \a
- /// OS. Return true if any error was found.
-
- virtual bool WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
+ /// OS.
+ virtual Error WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
/// Write the bundle from \a Input into \a OS.
- virtual void WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+ virtual Error WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
};
/// Handler for binary files. The bundled file will have the following format
@@ -224,7 +228,7 @@
~BinaryFileHandler() final {}
- void ReadHeader(MemoryBuffer ) final {
+ Error ReadHeader(MemoryBuffer ) final {
StringRef FC = Input.getBuffer();
// Initialize the current bundle with the end of the container.
@@ -233,16 +237,16 @@
// Check if buffer is smaller than magic string.
size_t ReadChars = sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1;
if (ReadChars > FC.size())
- return;
+ return Error::success();
// Check if no magic was found.
StringRef Magic(FC.data(), sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1);
if (!Magic.equals(OFFLOAD_BUNDLER_MAGIC_STR))
- return;
+ return Error::success();
// Read number of bundles.
if (ReadChars + 8 > FC.size())
- return;
+ return Error::success();
uint64_t NumberOfBundles = Read8byteIntegerFromBuffer(FC, ReadChars);
ReadChars += 8;
@@ -252,35 +256,35 @@
// Read offset.
if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
uint64_t Offset = Read8byteIntegerFromBuffer(FC, ReadChars);
ReadChars += 8;
// Read size.
if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
uint64_t Size = Read8byteIntegerFromBuffer(FC, ReadChars);
ReadChars += 8;
// Read triple size.
if (ReadChars + 8 > FC.size())
-return;
+
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev added inline comments. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:133 /// returned if there are no more bundles to be read. - virtual StringRef ReadBundleStart(MemoryBuffer ) = 0; + virtual Expected> + ReadBundleStart(MemoryBuffer ) = 0; ABataev wrote: > Do we really need an inner `Optional` here? The idea was to return `StringRef` with bundle name when we have still have bundles and `None` when there are no more bundles in the file (BTW comment has to be updated to describe this). But I can restore the old convention where empty bundle name means 'no more bundles' in the file. In terms of functionality that would be the same, though use of `Optional<...>` makes intentions more explicit)) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
ABataev added inline comments. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:133 /// returned if there are no more bundles to be read. - virtual StringRef ReadBundleStart(MemoryBuffer ) = 0; + virtual Expected> + ReadBundleStart(MemoryBuffer ) = 0; Do we really need an inner `Optional` here? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev added a comment. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev added a comment. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev added a comment. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev updated this revision to Diff 219742.
sdmitriev added a reviewer: Hahnfeld.
sdmitriev added a comment.
Rebase
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67031/new/
https://reviews.llvm.org/D67031
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
@@ -26,15 +26,17 @@
#include "llvm/Object/ObjectFile.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorOr.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
-#include "llvm/Support/raw_ostream.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/StringSaver.h"
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
#include
#include
#include
@@ -42,6 +44,7 @@
#include
#include
#include
+#include
using namespace llvm;
using namespace llvm::object;
@@ -122,35 +125,36 @@
/// Update the file handler with information from the header of the bundled
/// file
- virtual void ReadHeader(MemoryBuffer ) = 0;
+ virtual Error ReadHeader(MemoryBuffer ) = 0;
/// Read the marker of the next bundled to be read in the file. The triple of
/// the target associated with that bundle is returned. An empty string is
/// returned if there are no more bundles to be read.
- virtual StringRef ReadBundleStart(MemoryBuffer ) = 0;
+ virtual Expected>
+ ReadBundleStart(MemoryBuffer ) = 0;
/// Read the marker that closes the current bundle.
- virtual void ReadBundleEnd(MemoryBuffer ) = 0;
+ virtual Error ReadBundleEnd(MemoryBuffer ) = 0;
/// Read the current bundle and write the result into the stream \a OS.
- virtual void ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+ virtual Error ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
/// Write the header of the bundled file to \a OS based on the information
/// gathered from \a Inputs.
- virtual void WriteHeader(raw_fd_ostream ,
- ArrayRef> Inputs) = 0;
+ virtual Error WriteHeader(raw_fd_ostream ,
+ArrayRef> Inputs) = 0;
/// Write the marker that initiates a bundle for the triple \a TargetTriple to
/// \a OS.
- virtual void WriteBundleStart(raw_fd_ostream , StringRef TargetTriple) = 0;
+ virtual Error WriteBundleStart(raw_fd_ostream ,
+ StringRef TargetTriple) = 0;
/// Write the marker that closes a bundle for the triple \a TargetTriple to \a
- /// OS. Return true if any error was found.
-
- virtual bool WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
+ /// OS.
+ virtual Error WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
/// Write the bundle from \a Input into \a OS.
- virtual void WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+ virtual Error WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
};
/// Handler for binary files. The bundled file will have the following format
@@ -222,7 +226,7 @@
~BinaryFileHandler() final {}
- void ReadHeader(MemoryBuffer ) final {
+ Error ReadHeader(MemoryBuffer ) final {
StringRef FC = Input.getBuffer();
// Initialize the current bundle with the end of the container.
@@ -231,16 +235,16 @@
// Check if buffer is smaller than magic string.
size_t ReadChars = sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1;
if (ReadChars > FC.size())
- return;
+ return Error::success();
// Check if no magic was found.
StringRef Magic(FC.data(), sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1);
if (!Magic.equals(OFFLOAD_BUNDLER_MAGIC_STR))
- return;
+ return Error::success();
// Read number of bundles.
if (ReadChars + 8 > FC.size())
- return;
+ return Error::success();
uint64_t NumberOfBundles = Read8byteIntegerFromBuffer(FC, ReadChars);
ReadChars += 8;
@@ -250,35 +254,35 @@
// Read offset.
if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
uint64_t Offset = Read8byteIntegerFromBuffer(FC, ReadChars);
ReadChars += 8;
// Read size.
if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
uint64_t Size = Read8byteIntegerFromBuffer(FC, ReadChars);
ReadChars += 8;
// Read triple size.
if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
uint64_t TripleSize = Read8byteIntegerFromBuffer(FC, ReadChars);
ReadChars += 8;
// Read triple.
if (ReadChars + TripleSize > FC.size())
-
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev added a comment. Ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev added a comment. I think I have addressed all comments posted so far. Do you have more notes/comments/suggestions? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev marked an inline comment as done. sdmitriev added inline comments. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:41-44 -#include -#include -#include -#include Hahnfeld wrote: > sdmitriev wrote: > > Hahnfeld wrote: > > > sdmitriev wrote: > > > > Hahnfeld wrote: > > > > > sdmitriev wrote: > > > > > > Hahnfeld wrote: > > > > > > > The code still uses (in the order of marked includes) > > > > > > > * `std::unique_ptr` > > > > > > > * `std::string` > > > > > > > * `std::error_code` > > > > > > > * `std::vector`, > > > > > > > so I don't think these lines should be removed. Same goes for > > > > > > > `assert` and probably also `cstddef` / `cstdint` (`uint64_t`?) > > > > > > Right. Restored required system includes. > > > > > `vector` is still removed which is definitely used. Are you 100% sure > > > > > that `algorithm` and `cstddef` are not needed? > > > > I have replaced all uses of vector with SmallVector. > > > > > > > > > Are you 100% sure that algorithm and cstddef are not needed? > > > > > > > > Ok, I will add algorithm and cstddef back:) > > > If you want to replace `vector` by `SmallVector`, this must be its own > > > revision. > > Must? Can you show any document/link explaining this? > > I have no problem with doing that in a separate commit, not a big deal, > > just want to see why it has to be done that way. > This is common practice for reviews: Only one change per patch and split into > smaller patches if possible and logically appropriate. > > I can't quote a written requirement, but the Developer Policy section about > [[ https://llvm.org/docs/DeveloperPolicy.html#code-reviews | code reviews ]] > mentions splitting patches and there's a whole section about [[ > https://llvm.org/docs/DeveloperPolicy.html#incremental-development | > Incremental Development ]]. I have reverted vector -> SmallVector change. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev updated this revision to Diff 218776.
sdmitriev edited the summary of this revision.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67031/new/
https://reviews.llvm.org/D67031
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
@@ -17,6 +17,7 @@
#include "clang/Basic/Version.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSwitch.h"
@@ -25,15 +26,17 @@
#include "llvm/Object/ObjectFile.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorOr.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
-#include "llvm/Support/raw_ostream.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/StringSaver.h"
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
#include
#include
#include
@@ -42,6 +45,7 @@
#include
#include
#include
+#include
using namespace llvm;
using namespace llvm::object;
@@ -122,35 +126,36 @@
/// Update the file handler with information from the header of the bundled
/// file
- virtual void ReadHeader(MemoryBuffer ) = 0;
+ virtual Error ReadHeader(MemoryBuffer ) = 0;
/// Read the marker of the next bundled to be read in the file. The triple of
/// the target associated with that bundle is returned. An empty string is
/// returned if there are no more bundles to be read.
- virtual StringRef ReadBundleStart(MemoryBuffer ) = 0;
+ virtual Expected>
+ ReadBundleStart(MemoryBuffer ) = 0;
/// Read the marker that closes the current bundle.
- virtual void ReadBundleEnd(MemoryBuffer ) = 0;
+ virtual Error ReadBundleEnd(MemoryBuffer ) = 0;
/// Read the current bundle and write the result into the stream \a OS.
- virtual void ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+ virtual Error ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
/// Write the header of the bundled file to \a OS based on the information
/// gathered from \a Inputs.
- virtual void WriteHeader(raw_fd_ostream ,
- ArrayRef> Inputs) = 0;
+ virtual Error WriteHeader(raw_fd_ostream ,
+ArrayRef> Inputs) = 0;
/// Write the marker that initiates a bundle for the triple \a TargetTriple to
/// \a OS.
- virtual void WriteBundleStart(raw_fd_ostream , StringRef TargetTriple) = 0;
+ virtual Error WriteBundleStart(raw_fd_ostream ,
+ StringRef TargetTriple) = 0;
/// Write the marker that closes a bundle for the triple \a TargetTriple to \a
- /// OS. Return true if any error was found.
-
- virtual bool WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
+ /// OS.
+ virtual Error WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
/// Write the bundle from \a Input into \a OS.
- virtual void WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+ virtual Error WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
};
/// Handler for binary files. The bundled file will have the following format
@@ -222,7 +227,7 @@
~BinaryFileHandler() final {}
- void ReadHeader(MemoryBuffer ) final {
+ Error ReadHeader(MemoryBuffer ) final {
StringRef FC = Input.getBuffer();
// Initialize the current bundle with the end of the container.
@@ -231,16 +236,16 @@
// Check if buffer is smaller than magic string.
size_t ReadChars = sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1;
if (ReadChars > FC.size())
- return;
+ return Error::success();
// Check if no magic was found.
StringRef Magic(FC.data(), sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1);
if (!Magic.equals(OFFLOAD_BUNDLER_MAGIC_STR))
- return;
+ return Error::success();
// Read number of bundles.
if (ReadChars + 8 > FC.size())
- return;
+ return Error::success();
uint64_t NumberOfBundles = Read8byteIntegerFromBuffer(FC, ReadChars);
ReadChars += 8;
@@ -250,35 +255,35 @@
// Read offset.
if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
uint64_t Offset = Read8byteIntegerFromBuffer(FC, ReadChars);
ReadChars += 8;
// Read size.
if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
uint64_t Size = Read8byteIntegerFromBuffer(FC, ReadChars);
ReadChars += 8;
// Read triple size.
if (ReadChars + 8 >
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
Hahnfeld added inline comments. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:41-44 -#include -#include -#include -#include sdmitriev wrote: > Hahnfeld wrote: > > sdmitriev wrote: > > > Hahnfeld wrote: > > > > sdmitriev wrote: > > > > > Hahnfeld wrote: > > > > > > The code still uses (in the order of marked includes) > > > > > > * `std::unique_ptr` > > > > > > * `std::string` > > > > > > * `std::error_code` > > > > > > * `std::vector`, > > > > > > so I don't think these lines should be removed. Same goes for > > > > > > `assert` and probably also `cstddef` / `cstdint` (`uint64_t`?) > > > > > Right. Restored required system includes. > > > > `vector` is still removed which is definitely used. Are you 100% sure > > > > that `algorithm` and `cstddef` are not needed? > > > I have replaced all uses of vector with SmallVector. > > > > > > > Are you 100% sure that algorithm and cstddef are not needed? > > > > > > Ok, I will add algorithm and cstddef back:) > > If you want to replace `vector` by `SmallVector`, this must be its own > > revision. > Must? Can you show any document/link explaining this? > I have no problem with doing that in a separate commit, not a big deal, just > want to see why it has to be done that way. This is common practice for reviews: Only one change per patch and split into smaller patches if possible and logically appropriate. I can't quote a written requirement, but the Developer Policy section about [[ https://llvm.org/docs/DeveloperPolicy.html#code-reviews | code reviews ]] mentions splitting patches and there's a whole section about [[ https://llvm.org/docs/DeveloperPolicy.html#incremental-development | Incremental Development ]]. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev marked an inline comment as done. sdmitriev added inline comments. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:41-44 -#include -#include -#include -#include Hahnfeld wrote: > sdmitriev wrote: > > Hahnfeld wrote: > > > sdmitriev wrote: > > > > Hahnfeld wrote: > > > > > The code still uses (in the order of marked includes) > > > > > * `std::unique_ptr` > > > > > * `std::string` > > > > > * `std::error_code` > > > > > * `std::vector`, > > > > > so I don't think these lines should be removed. Same goes for > > > > > `assert` and probably also `cstddef` / `cstdint` (`uint64_t`?) > > > > Right. Restored required system includes. > > > `vector` is still removed which is definitely used. Are you 100% sure > > > that `algorithm` and `cstddef` are not needed? > > I have replaced all uses of vector with SmallVector. > > > > > Are you 100% sure that algorithm and cstddef are not needed? > > > > Ok, I will add algorithm and cstddef back:) > If you want to replace `vector` by `SmallVector`, this must be its own > revision. Must? Can you show any document/link explaining this? I have no problem with doing that in a separate commit, not a big deal, just want to see why it has to be done that way. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
Hahnfeld added a comment. Also, there should be a summary of the changes in here. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:41-44 -#include -#include -#include -#include sdmitriev wrote: > Hahnfeld wrote: > > sdmitriev wrote: > > > Hahnfeld wrote: > > > > The code still uses (in the order of marked includes) > > > > * `std::unique_ptr` > > > > * `std::string` > > > > * `std::error_code` > > > > * `std::vector`, > > > > so I don't think these lines should be removed. Same goes for `assert` > > > > and probably also `cstddef` / `cstdint` (`uint64_t`?) > > > Right. Restored required system includes. > > `vector` is still removed which is definitely used. Are you 100% sure that > > `algorithm` and `cstddef` are not needed? > I have replaced all uses of vector with SmallVector. > > > Are you 100% sure that algorithm and cstddef are not needed? > > Ok, I will add algorithm and cstddef back:) If you want to replace `vector` by `SmallVector`, this must be its own revision. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev updated this revision to Diff 218755.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67031/new/
https://reviews.llvm.org/D67031
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
@@ -17,6 +17,7 @@
#include "clang/Basic/Version.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSwitch.h"
@@ -25,15 +26,17 @@
#include "llvm/Object/ObjectFile.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorOr.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
-#include "llvm/Support/raw_ostream.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/StringSaver.h"
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
#include
#include
#include
@@ -41,7 +44,7 @@
#include
#include
#include
-#include
+#include
using namespace llvm;
using namespace llvm::object;
@@ -122,35 +125,36 @@
/// Update the file handler with information from the header of the bundled
/// file
- virtual void ReadHeader(MemoryBuffer ) = 0;
+ virtual Error ReadHeader(MemoryBuffer ) = 0;
/// Read the marker of the next bundled to be read in the file. The triple of
/// the target associated with that bundle is returned. An empty string is
/// returned if there are no more bundles to be read.
- virtual StringRef ReadBundleStart(MemoryBuffer ) = 0;
+ virtual Expected>
+ ReadBundleStart(MemoryBuffer ) = 0;
/// Read the marker that closes the current bundle.
- virtual void ReadBundleEnd(MemoryBuffer ) = 0;
+ virtual Error ReadBundleEnd(MemoryBuffer ) = 0;
/// Read the current bundle and write the result into the stream \a OS.
- virtual void ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+ virtual Error ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
/// Write the header of the bundled file to \a OS based on the information
/// gathered from \a Inputs.
- virtual void WriteHeader(raw_fd_ostream ,
- ArrayRef> Inputs) = 0;
+ virtual Error WriteHeader(raw_fd_ostream ,
+ArrayRef> Inputs) = 0;
/// Write the marker that initiates a bundle for the triple \a TargetTriple to
/// \a OS.
- virtual void WriteBundleStart(raw_fd_ostream , StringRef TargetTriple) = 0;
+ virtual Error WriteBundleStart(raw_fd_ostream ,
+ StringRef TargetTriple) = 0;
/// Write the marker that closes a bundle for the triple \a TargetTriple to \a
- /// OS. Return true if any error was found.
-
- virtual bool WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
+ /// OS.
+ virtual Error WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
/// Write the bundle from \a Input into \a OS.
- virtual void WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+ virtual Error WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
};
/// Handler for binary files. The bundled file will have the following format
@@ -222,7 +226,7 @@
~BinaryFileHandler() final {}
- void ReadHeader(MemoryBuffer ) final {
+ Error ReadHeader(MemoryBuffer ) final {
StringRef FC = Input.getBuffer();
// Initialize the current bundle with the end of the container.
@@ -231,16 +235,16 @@
// Check if buffer is smaller than magic string.
size_t ReadChars = sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1;
if (ReadChars > FC.size())
- return;
+ return Error::success();
// Check if no magic was found.
StringRef Magic(FC.data(), sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1);
if (!Magic.equals(OFFLOAD_BUNDLER_MAGIC_STR))
- return;
+ return Error::success();
// Read number of bundles.
if (ReadChars + 8 > FC.size())
- return;
+ return Error::success();
uint64_t NumberOfBundles = Read8byteIntegerFromBuffer(FC, ReadChars);
ReadChars += 8;
@@ -250,35 +254,35 @@
// Read offset.
if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
uint64_t Offset = Read8byteIntegerFromBuffer(FC, ReadChars);
ReadChars += 8;
// Read size.
if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
uint64_t Size = Read8byteIntegerFromBuffer(FC, ReadChars);
ReadChars += 8;
// Read triple size.
if (ReadChars + 8 > FC.size())
-return;
+
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev marked an inline comment as done. sdmitriev added inline comments. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:41-44 -#include -#include -#include -#include Hahnfeld wrote: > sdmitriev wrote: > > Hahnfeld wrote: > > > The code still uses (in the order of marked includes) > > > * `std::unique_ptr` > > > * `std::string` > > > * `std::error_code` > > > * `std::vector`, > > > so I don't think these lines should be removed. Same goes for `assert` > > > and probably also `cstddef` / `cstdint` (`uint64_t`?) > > Right. Restored required system includes. > `vector` is still removed which is definitely used. Are you 100% sure that > `algorithm` and `cstddef` are not needed? I have replaced all uses of vector with SmallVector. > Are you 100% sure that algorithm and cstddef are not needed? Ok, I will add algorithm and cstddef back:) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
Hahnfeld added inline comments. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:41-44 -#include -#include -#include -#include sdmitriev wrote: > Hahnfeld wrote: > > The code still uses (in the order of marked includes) > > * `std::unique_ptr` > > * `std::string` > > * `std::error_code` > > * `std::vector`, > > so I don't think these lines should be removed. Same goes for `assert` and > > probably also `cstddef` / `cstdint` (`uint64_t`?) > Right. Restored required system includes. `vector` is still removed which is definitely used. Are you 100% sure that `algorithm` and `cstddef` are not needed? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev marked an inline comment as done. sdmitriev added a comment. Any comments? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev updated this revision to Diff 218250.
sdmitriev added a comment.
Removed trailing '.' from error messages and added few additional changes for
better error handling.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67031/new/
https://reviews.llvm.org/D67031
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
@@ -17,6 +17,7 @@
#include "clang/Basic/Version.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSwitch.h"
@@ -25,23 +26,23 @@
#include "llvm/Object/ObjectFile.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorOr.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
-#include "llvm/Support/raw_ostream.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/StringSaver.h"
-#include
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
#include
-#include
#include
#include
#include
#include
-#include
+#include
using namespace llvm;
using namespace llvm::object;
@@ -122,35 +123,36 @@
/// Update the file handler with information from the header of the bundled
/// file
- virtual void ReadHeader(MemoryBuffer ) = 0;
+ virtual Error ReadHeader(MemoryBuffer ) = 0;
/// Read the marker of the next bundled to be read in the file. The triple of
/// the target associated with that bundle is returned. An empty string is
/// returned if there are no more bundles to be read.
- virtual StringRef ReadBundleStart(MemoryBuffer ) = 0;
+ virtual Expected>
+ ReadBundleStart(MemoryBuffer ) = 0;
/// Read the marker that closes the current bundle.
- virtual void ReadBundleEnd(MemoryBuffer ) = 0;
+ virtual Error ReadBundleEnd(MemoryBuffer ) = 0;
/// Read the current bundle and write the result into the stream \a OS.
- virtual void ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+ virtual Error ReadBundle(raw_fd_ostream , MemoryBuffer ) = 0;
/// Write the header of the bundled file to \a OS based on the information
/// gathered from \a Inputs.
- virtual void WriteHeader(raw_fd_ostream ,
- ArrayRef> Inputs) = 0;
+ virtual Error WriteHeader(raw_fd_ostream ,
+ArrayRef> Inputs) = 0;
/// Write the marker that initiates a bundle for the triple \a TargetTriple to
/// \a OS.
- virtual void WriteBundleStart(raw_fd_ostream , StringRef TargetTriple) = 0;
+ virtual Error WriteBundleStart(raw_fd_ostream ,
+ StringRef TargetTriple) = 0;
/// Write the marker that closes a bundle for the triple \a TargetTriple to \a
- /// OS. Return true if any error was found.
-
- virtual bool WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
+ /// OS.
+ virtual Error WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
/// Write the bundle from \a Input into \a OS.
- virtual void WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
+ virtual Error WriteBundle(raw_fd_ostream , MemoryBuffer ) = 0;
};
/// Handler for binary files. The bundled file will have the following format
@@ -222,7 +224,7 @@
~BinaryFileHandler() final {}
- void ReadHeader(MemoryBuffer ) final {
+ Error ReadHeader(MemoryBuffer ) final {
StringRef FC = Input.getBuffer();
// Initialize the current bundle with the end of the container.
@@ -231,16 +233,16 @@
// Check if buffer is smaller than magic string.
size_t ReadChars = sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1;
if (ReadChars > FC.size())
- return;
+ return Error::success();
// Check if no magic was found.
StringRef Magic(FC.data(), sizeof(OFFLOAD_BUNDLER_MAGIC_STR) - 1);
if (!Magic.equals(OFFLOAD_BUNDLER_MAGIC_STR))
- return;
+ return Error::success();
// Read number of bundles.
if (ReadChars + 8 > FC.size())
- return;
+ return Error::success();
uint64_t NumberOfBundles = Read8byteIntegerFromBuffer(FC, ReadChars);
ReadChars += 8;
@@ -250,35 +252,35 @@
// Read offset.
if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
uint64_t Offset = Read8byteIntegerFromBuffer(FC, ReadChars);
ReadChars += 8;
// Read size.
if (ReadChars + 8 > FC.size())
-return;
+return Error::success();
uint64_t Size = Read8byteIntegerFromBuffer(FC,
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev marked an inline comment as done. sdmitriev added inline comments. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:906 +Msg << ", unknown target triple '" << Triple << "'"; + reportError(createStringError(errc::invalid_argument, Msg.str() + ".")); } MaskRay wrote: > I think trailing full stop is uncommon in error messages. Maybe., but all error messages in this tool seem to be consistent in that sense)) Do you suggest removing trailing '.' from all error messages? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
MaskRay added inline comments. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:906 +Msg << ", unknown target triple '" << Triple << "'"; + reportError(createStringError(errc::invalid_argument, Msg.str() + ".")); } I think trailing full stop is uncommon in error messages. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev marked an inline comment as done. sdmitriev added inline comments. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:41-44 -#include -#include -#include -#include Hahnfeld wrote: > The code still uses (in the order of marked includes) > * `std::unique_ptr` > * `std::string` > * `std::error_code` > * `std::vector`, > so I don't think these lines should be removed. Same goes for `assert` and > probably also `cstddef` / `cstdint` (`uint64_t`?) Right. Restored required system includes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements
sdmitriev updated this revision to Diff 218224.
sdmitriev retitled this revision from "[Clang][Bundler] Error reporting
improvements [NFC]" to "[Clang][Bundler] Error reporting improvements".
sdmitriev added a comment.
Addressed review comments.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67031/new/
https://reviews.llvm.org/D67031
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
@@ -17,6 +17,7 @@
#include "clang/Basic/Version.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSwitch.h"
@@ -25,23 +26,23 @@
#include "llvm/Object/ObjectFile.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorOr.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
-#include "llvm/Support/raw_ostream.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/StringSaver.h"
-#include
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
#include
-#include
#include
#include
#include
#include
-#include
+#include
using namespace llvm;
using namespace llvm::object;
@@ -98,6 +99,9 @@
/// Path to the current binary.
static std::string BundlerExecutable;
+/// Saved argv[0].
+static StringRef Argv0;
+
/// Obtain the offload kind and real machine triple out of the target
/// information specified by the user.
static void getOffloadKindAndTriple(StringRef Target, StringRef ,
@@ -113,6 +117,15 @@
return OffloadKind == "host";
}
+/// Error reporting functions.
+static void reportError(Error E) {
+ logAllUnhandledErrors(std::move(E), WithColor::error(errs(), Argv0));
+}
+
+static void reportFileError(StringRef File, std::error_code EC) {
+ reportError(createFileError(File, EC));
+}
+
/// Generic file handler interface.
class FileHandler {
public:
@@ -146,7 +159,6 @@
/// Write the marker that closes a bundle for the triple \a TargetTriple to \a
/// OS. Return true if any error was found.
-
virtual bool WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
/// Write the bundle from \a Input into \a OS.
@@ -327,7 +339,7 @@
unsigned Idx = 0;
for (auto : TargetNames) {
- MemoryBuffer = *Inputs[Idx++].get();
+ MemoryBuffer = *Inputs[Idx++];
// Bundle offset.
Write8byteIntegerToBuffer(OS, HeaderSize);
// Size of the bundle (adds to the next bundle's offset)
@@ -467,7 +479,8 @@
ErrorOr Objcopy = sys::findProgramByName(
"llvm-objcopy", sys::path::parent_path(BundlerExecutable));
if (!Objcopy) {
- errs() << "error: unable to find 'llvm-objcopy' in path.\n";
+ reportError(createStringError(Objcopy.getError(),
+"unable to find 'llvm-objcopy' in path."));
return true;
}
@@ -489,13 +502,14 @@
// If the user asked for the commands to be printed out, we do that instead
// of executing it.
if (PrintExternalCommands) {
- errs() << "\"" << Objcopy.get() << "\"";
+ errs() << "\"" << *Objcopy << "\"";
for (StringRef Arg : drop_begin(ObjcopyArgs, 1))
errs() << " \"" << Arg << "\"";
errs() << "\n";
} else {
- if (sys::ExecuteAndWait(Objcopy.get(), ObjcopyArgs)) {
-errs() << "error: llvm-objcopy tool failed.\n";
+ if (sys::ExecuteAndWait(*Objcopy, ObjcopyArgs)) {
+reportError(createStringError(inconvertibleErrorCode(),
+ "'llvm-objcopy' tool failed."));
return true;
}
}
@@ -622,13 +636,11 @@
// We only support regular object files. If this is not an object file,
// default to the binary handler. The handler will be owned by the client of
// this function.
- std::unique_ptr Obj(
- dyn_cast(BinaryOrErr.get().release()));
-
- if (!Obj)
-return new BinaryFileHandler();
-
- return new ObjectFileHandler(std::move(Obj));
+ if (auto *Obj = dyn_cast(BinaryOrErr->get())) {
+ BinaryOrErr->release();
+ return new ObjectFileHandler(std::unique_ptr(Obj));
+ }
+ return new BinaryFileHandler();
}
/// Return an appropriate handler given the input files and options.
@@ -650,7 +662,10 @@
if (FilesType == "ast")
return new BinaryFileHandler();
- errs() << "error: invalid file type specified.\n";
+ reportError(
+ createStringError(errc::invalid_argument,
+"'" + FilesType + "':
[PATCH] D67031: [Clang][Bundler] Error reporting improvements [NFC]
Hahnfeld added a comment. This changes error messages, so I'd say it's not NFC. Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:41-44 -#include -#include -#include -#include The code still uses (in the order of marked includes) * `std::unique_ptr` * `std::string` * `std::error_code` * `std::vector`, so I don't think these lines should be removed. Same goes for `assert` and probably also `cstddef` / `cstdint` (`uint64_t`?) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67031/new/ https://reviews.llvm.org/D67031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67031: [Clang][Bundler] Error reporting improvements [NFC]
sdmitriev created this revision.
sdmitriev added a reviewer: ABataev.
Herald added a reviewer: alexshap.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Repository:
rC Clang
https://reviews.llvm.org/D67031
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
@@ -25,23 +25,17 @@
#include "llvm/Object/ObjectFile.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorOr.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
-#include "llvm/Support/raw_ostream.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/StringSaver.h"
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
using namespace llvm;
using namespace llvm::object;
@@ -98,6 +92,9 @@
/// Path to the current binary.
static std::string BundlerExecutable;
+/// Saved argv[0].
+static StringRef Argv0;
+
/// Obtain the offload kind and real machine triple out of the target
/// information specified by the user.
static void getOffloadKindAndTriple(StringRef Target, StringRef ,
@@ -113,6 +110,15 @@
return OffloadKind == "host";
}
+/// Error reporting functions.
+static void reportError(Error E) {
+ logAllUnhandledErrors(std::move(E), WithColor::error(errs(), Argv0));
+}
+
+static void reportFileError(StringRef File, std::error_code EC) {
+ reportError(createFileError(File, EC));
+}
+
/// Generic file handler interface.
class FileHandler {
public:
@@ -146,7 +152,6 @@
/// Write the marker that closes a bundle for the triple \a TargetTriple to \a
/// OS. Return true if any error was found.
-
virtual bool WriteBundleEnd(raw_fd_ostream , StringRef TargetTriple) = 0;
/// Write the bundle from \a Input into \a OS.
@@ -327,7 +332,7 @@
unsigned Idx = 0;
for (auto : TargetNames) {
- MemoryBuffer = *Inputs[Idx++].get();
+ MemoryBuffer = *Inputs[Idx++];
// Bundle offset.
Write8byteIntegerToBuffer(OS, HeaderSize);
// Size of the bundle (adds to the next bundle's offset)
@@ -467,7 +472,8 @@
ErrorOr Objcopy = sys::findProgramByName(
"llvm-objcopy", sys::path::parent_path(BundlerExecutable));
if (!Objcopy) {
- errs() << "error: unable to find 'llvm-objcopy' in path.\n";
+ reportError(createStringError(Objcopy.getError(),
+"unable to find 'llvm-objcopy' in path."));
return true;
}
@@ -489,13 +495,14 @@
// If the user asked for the commands to be printed out, we do that instead
// of executing it.
if (PrintExternalCommands) {
- errs() << "\"" << Objcopy.get() << "\"";
+ errs() << "\"" << *Objcopy << "\"";
for (StringRef Arg : drop_begin(ObjcopyArgs, 1))
errs() << " \"" << Arg << "\"";
errs() << "\n";
} else {
- if (sys::ExecuteAndWait(Objcopy.get(), ObjcopyArgs)) {
-errs() << "error: llvm-objcopy tool failed.\n";
+ if (sys::ExecuteAndWait(*Objcopy, ObjcopyArgs)) {
+reportError(createStringError(inconvertibleErrorCode(),
+ "'llvm-objcopy' tool failed."));
return true;
}
}
@@ -622,13 +629,11 @@
// We only support regular object files. If this is not an object file,
// default to the binary handler. The handler will be owned by the client of
// this function.
- std::unique_ptr Obj(
- dyn_cast(BinaryOrErr.get().release()));
-
- if (!Obj)
-return new BinaryFileHandler();
-
- return new ObjectFileHandler(std::move(Obj));
+ if (auto *Obj = dyn_cast(BinaryOrErr->get())) {
+ BinaryOrErr->release();
+ return new ObjectFileHandler(std::unique_ptr(Obj));
+ }
+ return new BinaryFileHandler();
}
/// Return an appropriate handler given the input files and options.
@@ -650,7 +655,10 @@
if (FilesType == "ast")
return new BinaryFileHandler();
- errs() << "error: invalid file type specified.\n";
+ reportError(
+ createStringError(errc::invalid_argument,
+"'" + FilesType + "': invalid file type specified."));
+
return nullptr;
}
@@ -662,7 +670,7 @@
raw_fd_ostream OutputFile(OutputFileNames.front(), EC, sys::fs::OF_None);
if (EC) {
-errs() << "error: Can't open file " << OutputFileNames.front() << ".\n";
+reportFileError(OutputFileNames.front(), EC);
return true;
}
@@ -675,31 +683,31 @@
