Skip to content

Add --dump-offload-bundle option to llvm-objcopy #143347

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Add --dump-offload-bundle option to llvm-objcopy #143347

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0f0030c
Add --dump-offload-bundle option to llvm-objcopy
david-salinas Aug 28, 2024
bc29780
address PR review comments.
Jul 21, 2025
1447801
Code cleanup from PR comments
Aug 12, 2025
a454551
Resolve PR comments, add new Error tests.
Aug 21, 2025
78faec5
Merge branch 'main' into extend-offloading-api-llvm-objcopy
david-salinas Aug 21, 2025
e493a07
Merge branch 'main' into extend-offloading-api-llvm-objcopy
david-salinas Sep 10, 2025
448bfce
Merge branch 'main' into extend-offloading-api-llvm-objcopy
david-salinas Sep 10, 2025
79b6ace
clang-format cleanup
david-salinas Sep 10, 2025
47dbdbd
resolve build error
david-salinas Sep 15, 2025
f88b75e
cleanup to remove warnings
david-salinas Sep 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions llvm/docs/CommandGuide/llvm-objcopy.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,12 @@ multiple file formats.
For MachO objects, ``<section>`` must be formatted as
``<segment name>,<section name>``.

.. option:: --dump-offload-bundle=<URI>

Dump the HIP Offload Bundle entry specified by the URI syntax given, into a
code object file.


.. option:: --enable-deterministic-archives, -D

Enable deterministic mode when copying archives, i.e. use 0 for archive member
Expand Down
2 changes: 2 additions & 0 deletions llvm/include/llvm/ObjCopy/CommonConfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,8 @@ struct CommonConfig {
bool StripUnneeded = false;
bool Weaken = false;
bool DecompressDebugSections = false;
bool DumpOffloadBundle = false;
bool NeedPositional = true;

DebugCompressionType CompressionType = DebugCompressionType::None;

Expand Down
2 changes: 1 addition & 1 deletion llvm/include/llvm/Object/OffloadBundle.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ struct OffloadBundleURI {
OffsetStr.getAsInteger(10, O);
Str = Str.drop_front(OffsetStr.size());

if (Str.consume_front("&size="))
if (!Str.consume_front("&size="))
return createStringError(object_error::parse_failed,
"Reading 'size' in URI");

Expand Down
21 changes: 21 additions & 0 deletions llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,27 @@ static Error dumpSectionToFile(StringRef SecName, StringRef Filename,
"section '%s' not found", SecName.str().c_str());
}

static Error dumpRawDataURIToFile(StringRef Filename, int64_t Offset,
int64_t Size, ObjectFile &Obj) {
SmallString<2048> NameBuf;
raw_svector_ostream OutputFileName(NameBuf);
OutputFileName << Obj.getFileName().str() << "-offset" << Offset << "-size"
<< Size << ".co";

Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
FileOutputBuffer::create(OutputFileName.str(), Size);

if (!BufferOrErr)
return BufferOrErr.takeError();
Copy link
Collaborator

@jh7370 jh7370 Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests that show the error handling here and elsewhere in this change is correct.

Copy link
Contributor Author

@david-salinas david-salinas Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function does not get called. I must have left in from an earlier draft. I'm going to remove it. However ... in trying to test this error case, and realizing that we do call "extractOffloadBundleByURI" in the API source in OffloadBundler.cpp, I uncovered that the API function throws an exception/error that isn't handled. So I'll fix that and add an error check (or two) in the test case "dump-offload-bundle.test".


MemoryBufferRef Input = Obj.getMemoryBufferRef();
std::unique_ptr<FileOutputBuffer> Buf = std::move(*BufferOrErr);
std::copy(Input.getBufferStart(), Input.getBufferStart() + Size,
Buf->getBufferStart());
Copy link
Contributor

@jhuber6 jhuber6 Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, you can use llvm::copy to avoid the iterators.

Copy link
Contributor Author

@david-salinas david-salinas Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to change this to use llvm::copy but because we need to do a small calculation to get "end" it would actually make this code more complicated to call llvm::copy, and llvm::copy just calls std:copy anyway.


return Buf->commit();
}

Error Object::compressOrDecompressSections(const CommonConfig &Config) {
// Build a list of sections we are going to replace.
// We can't call `addSection` while iterating over sections,
Expand Down
60 changes: 60 additions & 0 deletions llvm/test/tools/llvm-objcopy/ELF/dump-offload-bundle.test
View file
Open in desktop

Large diffs are not rendered by default.

132 changes: 79 additions & 53 deletions llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
#include "llvm/ObjCopy/ConfigManager.h"
#include "llvm/ObjCopy/MachO/MachOConfig.h"
#include "llvm/Object/Binary.h"
#include "llvm/Object/OffloadBinary.h"
#include "llvm/Object/OffloadBundle.h"
#include "llvm/Option/Arg.h"
#include "llvm/Option/ArgList.h"
#include "llvm/Support/CRC.h"
Expand Down Expand Up @@ -284,6 +286,11 @@ static Expected<uint8_t> parseVisibilityType(StringRef VisType) {
return type;
}

static void llvm::objcopy::parseDumpOffloadBundle(StringRef URI) {
if (Error Err = object::extractOffloadBundleByURI(URI))
outs() << "Failed to extract from URI.";
Copy link
Contributor

@jhuber6 jhuber6 Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

outs() or err()?

Copy link
Contributor

@arsenm arsenm Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error messages should start with lowercase and not end in a period https://llvm.org/docs/CodingStandards.html#error-and-warning-messages

Copy link
Contributor Author

@david-salinas david-salinas Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Collaborator

@jh7370 jh7370 Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be either outs() or errs(): the error should be reported using llvm-objcopy's existing error reporting mechanism, by passing an Error up the stack.

Copy link
Contributor Author

@david-salinas david-salinas Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be either outs() or errs(): the error should be reported using llvm-objcopy's existing error reporting mechanism, by passing an Error up the stack.

agreed.

}

namespace {
struct TargetInfo {
FileFormat Format;
Expand Down Expand Up @@ -727,34 +734,45 @@ objcopy::parseObjcopyOptions(ArrayRef<const char *> ArgsArr,

SmallVector<const char *, 2> Positional;

ConfigManager ConfigMgr;
CommonConfig &Config = ConfigMgr.Common;
COFFConfig &COFFConfig = ConfigMgr.COFF;
ELFConfig &ELFConfig = ConfigMgr.ELF;
MachOConfig &MachOConfig = ConfigMgr.MachO;

if (InputArgs.hasArg(OBJCOPY_dump_offload_bundle))
Config.NeedPositional = false;

for (auto *Arg : InputArgs.filtered(OBJCOPY_UNKNOWN))
return createStringError(errc::invalid_argument, "unknown argument '%s'",
Arg->getAsString(InputArgs).c_str());

for (auto *Arg : InputArgs.filtered(OBJCOPY_INPUT))
Positional.push_back(Arg->getValue());

if (Positional.empty())
if (Positional.empty() && Config.NeedPositional)
return createStringError(errc::invalid_argument, "no input file specified");

if (Positional.size() > 2)
if (Positional.size() > 2 && Config.NeedPositional)
return createStringError(errc::invalid_argument,
"too many positional arguments");

ConfigManager ConfigMgr;
CommonConfig &Config = ConfigMgr.Common;
COFFConfig &COFFConfig = ConfigMgr.COFF;
ELFConfig &ELFConfig = ConfigMgr.ELF;
MachOConfig &MachOConfig = ConfigMgr.MachO;
Config.InputFilename = Positional[0];
Config.OutputFilename = Positional[Positional.size() == 1 ? 0 : 1];
if (InputArgs.hasArg(OBJCOPY_target) &&
(InputArgs.hasArg(OBJCOPY_input_target) ||
InputArgs.hasArg(OBJCOPY_output_target)))
return createStringError(
errc::invalid_argument,
"--target cannot be used with --input-target or --output-target");
if (Arg *A = InputArgs.getLastArg(OBJCOPY_dump_offload_bundle)) {
for (StringRef URIStr : llvm::split(A->getValue(), ",")) {
llvm::objcopy::parseDumpOffloadBundle(URIStr);
}
Copy link
Contributor

@jhuber6 jhuber6 Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (StringRef URIStr : llvm::split(A->getValue(), ",")) {
llvm::objcopy::parseDumpOffloadBundle(URIStr);
}
for (StringRef URIStr : llvm::split(A->getValue(), ","))
llvm::objcopy::parseDumpOffloadBundle(URIStr);

Copy link
Contributor Author

@david-salinas david-salinas Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

}

if (Config.NeedPositional) {
Config.InputFilename = Positional[0];
Config.OutputFilename = Positional[Positional.size() == 1 ? 0 : 1];
if (InputArgs.hasArg(OBJCOPY_target) &&
(InputArgs.hasArg(OBJCOPY_input_target) ||
InputArgs.hasArg(OBJCOPY_output_target)))
return createStringError(
errc::invalid_argument,
"--target cannot be used with --input-target or --output-target");
}
if (InputArgs.hasArg(OBJCOPY_regex) && InputArgs.hasArg(OBJCOPY_wildcard))
return createStringError(errc::invalid_argument,
"--regex and --wildcard are incompatible");
Expand Down Expand Up @@ -1417,25 +1435,26 @@ objcopy::parseInstallNameToolOptions(ArrayRef<const char *> ArgsArr) {
Arg->getAsString(InputArgs).c_str());
for (auto *Arg : InputArgs.filtered(INSTALL_NAME_TOOL_INPUT))
Positional.push_back(Arg->getValue());
if (Positional.empty())
if (Positional.empty() && Config.NeedPositional)
return createStringError(errc::invalid_argument, "no input file specified");
if (Positional.size() > 1)
if (Positional.size() > 1 && Config.NeedPositional)
return createStringError(
errc::invalid_argument,
"llvm-install-name-tool expects a single input file");
Config.InputFilename = Positional[0];
Config.OutputFilename = Positional[0];

Expected<OwningBinary<Binary>> BinaryOrErr =
createBinary(Config.InputFilename);
if (!BinaryOrErr)
return createFileError(Config.InputFilename, BinaryOrErr.takeError());
auto *Binary = (*BinaryOrErr).getBinary();
if (!Binary->isMachO() && !Binary->isMachOUniversalBinary())
return createStringError(errc::invalid_argument,
"input file: %s is not a Mach-O file",
Config.InputFilename.str().c_str());

if (Config.NeedPositional) {
Copy link
Collaborator

@jh7370 jh7370 Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code refers to llvm-install-name-tool. The new option shouldn't impact it in any way.

Config.InputFilename = Positional[0];
Config.OutputFilename = Positional[0];

Expected<OwningBinary<Binary>> BinaryOrErr =
createBinary(Config.InputFilename);
if (!BinaryOrErr)
return createFileError(Config.InputFilename, BinaryOrErr.takeError());
auto *Binary = (*BinaryOrErr).getBinary();
Copy link
Contributor

@jhuber6 jhuber6 Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
auto *Binary = (*BinaryOrErr).getBinary();
auto &Binary = *BinaryOrErr->getBinary();

Copy link
Contributor Author

@david-salinas david-salinas Jun 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually existing code that I'm just indenting because I had to add "if (Config.NeedsPositional)". I need to do this because llvm-objcopy is set up to always expect a source file argument unless the "NeedsPositional" config setting is negative/false. And with the new --dump-offload-bundle option I'm adding, it takes a URI argument which contains the source file argument. So if the --dump-offload-bundle option is specified I set "NeedsPositional" to false.

Copy link
Collaborator

@jh7370 jh7370 Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the above comment: this code isn't used by llvm-objcopy itself.

Copy link
Contributor Author

@david-salinas david-salinas Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, then no need for the NeedsPositional check ...

if (!Binary->isMachO() && !Binary->isMachOUniversalBinary())
return createStringError(errc::invalid_argument,
"input file: %s is not a Mach-O file",
Config.InputFilename.str().c_str());
}
DC.CopyConfigs.push_back(std::move(ConfigMgr));
return std::move(DC);
}
Expand Down Expand Up @@ -1474,13 +1493,16 @@ objcopy::parseBitcodeStripOptions(ArrayRef<const char *> ArgsArr,
Arg->getAsString(InputArgs).c_str());

SmallVector<StringRef, 2> Positional;
for (auto *Arg : InputArgs.filtered(BITCODE_STRIP_INPUT))
Positional.push_back(Arg->getValue());
if (Positional.size() > 1)
return createStringError(errc::invalid_argument,
"llvm-bitcode-strip expects a single input file");
assert(!Positional.empty());
Config.InputFilename = Positional[0];
if (Config.NeedPositional) {
for (auto *Arg : InputArgs.filtered(BITCODE_STRIP_INPUT))
Positional.push_back(Arg->getValue());
if (Positional.size() > 1)
return createStringError(
errc::invalid_argument,
"llvm-bitcode-strip expects a single input file");
assert(!Positional.empty());
Config.InputFilename = Positional[0];
}

if (!InputArgs.hasArg(BITCODE_STRIP_output)) {
return createStringError(errc::invalid_argument,
Expand Down Expand Up @@ -1542,27 +1564,31 @@ objcopy::parseStripOptions(ArrayRef<const char *> RawArgsArr,
exit(0);
}

SmallVector<StringRef, 2> Positional;
for (auto *Arg : InputArgs.filtered(STRIP_UNKNOWN))
return createStringError(errc::invalid_argument, "unknown argument '%s'",
Arg->getAsString(InputArgs).c_str());
for (auto *Arg : InputArgs.filtered(STRIP_INPUT))
Positional.push_back(Arg->getValue());
std::copy(DashDash, RawArgsArr.end(), std::back_inserter(Positional));

if (Positional.empty())
return createStringError(errc::invalid_argument, "no input file specified");

if (Positional.size() > 1 && InputArgs.hasArg(STRIP_output))
return createStringError(
errc::invalid_argument,
"multiple input files cannot be used in combination with -o");

ConfigManager ConfigMgr;
CommonConfig &Config = ConfigMgr.Common;
ELFConfig &ELFConfig = ConfigMgr.ELF;
MachOConfig &MachOConfig = ConfigMgr.MachO;

SmallVector<StringRef, 2> Positional;
if (Config.NeedPositional) {
for (auto *Arg : InputArgs.filtered(STRIP_UNKNOWN))
return createStringError(errc::invalid_argument, "unknown argument '%s'",
Arg->getAsString(InputArgs).c_str());
for (auto *Arg : InputArgs.filtered(STRIP_INPUT))
Positional.push_back(Arg->getValue());
std::copy(DashDash, RawArgsArr.end(), std::back_inserter(Positional));

if (Positional.empty())
return createStringError(errc::invalid_argument,
"no input file specified");

if (Positional.size() > 1 && InputArgs.hasArg(STRIP_output)) {
return createStringError(
errc::invalid_argument,
"multiple input files cannot be used in combination with -o");
}
}

if (InputArgs.hasArg(STRIP_regex) && InputArgs.hasArg(STRIP_wildcard))
return createStringError(errc::invalid_argument,
"--regex and --wildcard are incompatible");
Expand Down
5 changes: 5 additions & 0 deletions llvm/tools/llvm-objcopy/ObjcopyOptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ parseBitcodeStripOptions(ArrayRef<const char *> ArgsArr,
Expected<DriverConfig>
parseStripOptions(ArrayRef<const char *> ArgsArr,
llvm::function_ref<Error(Error)> ErrorCallback);

// parseDumpURI reads a URI as a string, and extracts the raw memory into a
// code object file named from the URI string given
static void parseDumpOffloadBundle(StringRef URI);

} // namespace objcopy
} // namespace llvm

Expand Down
3 changes: 3 additions & 0 deletions llvm/tools/llvm-objcopy/ObjcopyOpts.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,9 @@ defm dump_section
: Eq<"dump-section",
"Dump contents of section named <section> into file <file>">,
MetaVarName<"section=file">;

defm dump_offload_bundle : Eq<"dump-offload-bundle", "Dump the contents specified by URI">;

defm prefix_symbols
: Eq<"prefix-symbols", "Add <prefix> to the start of every symbol name">,
MetaVarName<"prefix">;
Expand Down
Loading
Loading