Skip to content
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.

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

[llvm-objcopy] Fix prints wrong path when dump-section output path doesn't exist #125345

Merged
2 changes: 2 additions & 0 deletions llvm/docs/ReleaseNotes.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,8 @@ Changes to the Debug Info
Changes to the LLVM tools
---------------------------------

- llvm-objcopy now correct path in error message when section output path doesn't exist.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- llvm-objcopy now correct path in error message when section output path doesn't exist.
- llvm-objcopy now prints the correct file path in the error message when a section specified by --dump-section doesn't exist.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hang on, sorry, I might have got mixed up about which path is being fixed...

Try this:

"llvm-objcopy now prints the correct file path in the error message when the output file specified by --dump-section cannot be opened."

Copy link
Member Author

Choose a reason for hiding this comment

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

LGTM, with the suggested change to the release note to be more specific and grammatically natural/correct. Do you need me to merge for you?

I can merge the PR, once merged I can cheery pick it to release branch

Copy link
Member Author

Choose a reason for hiding this comment

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

"llvm-objcopy now prints the correct file path in the error message when the output file specified by --dump-section cannot be opened."

I updated the release note with this message

Copy link
Member

Choose a reason for hiding this comment

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

llvm-objcopy now fixes ... when the --dump-section specified output file cannot be opened


Changes to LLDB
---------------------------------

Expand Down
59 changes: 30 additions & 29 deletions llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -186,27 +186,28 @@ static std::unique_ptr<Writer> createWriter(const CommonConfig &Config,
}

static Error dumpSectionToFile(StringRef SecName, StringRef Filename,
Object &Obj) {
StringRef InputFilename, Object &Obj) {
for (auto &Sec : Obj.sections()) {
if (Sec.Name == SecName) {
if (Sec.Type == SHT_NOBITS)
return createStringError(object_error::parse_failed,
"cannot dump section '%s': it has no contents",
SecName.str().c_str());
return createFileError(InputFilename, object_error::parse_failed,
"cannot dump section '%s': it has no contents",
SecName.str().c_str());
Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
FileOutputBuffer::create(Filename, Sec.OriginalData.size());
if (!BufferOrErr)
return BufferOrErr.takeError();
return createFileError(Filename, BufferOrErr.takeError());
std::unique_ptr<FileOutputBuffer> Buf = std::move(*BufferOrErr);
std::copy(Sec.OriginalData.begin(), Sec.OriginalData.end(),
Buf->getBufferStart());
if (Error E = Buf->commit())
return E;
return createFileError(Filename, std::move(E));
return Error::success();
}
}
return createStringError(object_error::parse_failed, "section '%s' not found",
SecName.str().c_str());

return createFileError(InputFilename, object_error::parse_failed,
"section '%s' not found", SecName.str().c_str());
}

Error Object::compressOrDecompressSections(const CommonConfig &Config) {
Expand Down Expand Up @@ -798,7 +799,8 @@ static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
StringRef SectionName;
StringRef FileName;
std::tie(SectionName, FileName) = Flag.split('=');
if (Error E = dumpSectionToFile(SectionName, FileName, Obj))
if (Error E =
dumpSectionToFile(SectionName, FileName, Config.InputFilename, Obj))
return E;
}

Expand All @@ -807,10 +809,10 @@ static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
// us to avoid reporting the inappropriate errors about removing symbols
// named in relocations.
if (Error E = replaceAndRemoveSections(Config, ELFConfig, Obj))
return E;
return createFileError(Config.InputFilename, std::move(E));

if (Error E = updateAndRemoveSymbols(Config, ELFConfig, Obj))
return E;
return createFileError(Config.InputFilename, std::move(E));

if (!Config.SetSectionAlignment.empty()) {
for (SectionBase &Sec : Obj.sections()) {
Expand All @@ -826,17 +828,17 @@ static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
if (Config.ChangeSectionLMAValAll > 0 &&
Seg.PAddr > std::numeric_limits<uint64_t>::max() -
Config.ChangeSectionLMAValAll) {
return createStringError(
errc::invalid_argument,
return createFileError(
Config.InputFilename, errc::invalid_argument,
"address 0x" + Twine::utohexstr(Seg.PAddr) +
" cannot be increased by 0x" +
Twine::utohexstr(Config.ChangeSectionLMAValAll) +
". The result would overflow");
} else if (Config.ChangeSectionLMAValAll < 0 &&
Seg.PAddr < std::numeric_limits<uint64_t>::min() -
Config.ChangeSectionLMAValAll) {
return createStringError(
errc::invalid_argument,
return createFileError(
Config.InputFilename, errc::invalid_argument,
"address 0x" + Twine::utohexstr(Seg.PAddr) +
" cannot be decreased by 0x" +
Twine::utohexstr(std::abs(Config.ChangeSectionLMAValAll)) +
Expand All @@ -849,10 +851,9 @@ static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,

if (!Config.ChangeSectionAddress.empty()) {
if (Obj.Type != ELF::ET_REL)
return createStringError(
object_error::invalid_file_type,
return createFileError(
Config.InputFilename, object_error::invalid_file_type,
"cannot change section address in a non-relocatable file");

StringMap<AddressUpdate> SectionsToUpdateAddress;
for (const SectionPatternAddressUpdate &PatternUpdate :
make_range(Config.ChangeSectionAddress.rbegin(),
Expand All @@ -863,8 +864,8 @@ static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
.second) {
if (PatternUpdate.Update.Kind == AdjustKind::Subtract &&
Sec.Addr < PatternUpdate.Update.Value) {
return createStringError(
errc::invalid_argument,
return createFileError(
Config.InputFilename, errc::invalid_argument,
"address 0x" + Twine::utohexstr(Sec.Addr) +
" cannot be decreased by 0x" +
Twine::utohexstr(PatternUpdate.Update.Value) +
Expand All @@ -873,8 +874,8 @@ static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
if (PatternUpdate.Update.Kind == AdjustKind::Add &&
Sec.Addr > std::numeric_limits<uint64_t>::max() -
PatternUpdate.Update.Value) {
return createStringError(
errc::invalid_argument,
return createFileError(
Config.InputFilename, errc::invalid_argument,
"address 0x" + Twine::utohexstr(Sec.Addr) +
" cannot be increased by 0x" +
Twine::utohexstr(PatternUpdate.Update.Value) +
Expand Down Expand Up @@ -909,7 +910,7 @@ static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
if (!ELFConfig.NotesToRemove.empty()) {
if (Error Err =
removeNotes(Obj, E, ELFConfig.NotesToRemove, Config.ErrorCallback))
return Err;
return createFileError(Config.InputFilename, std::move(Err));
}

for (const NewSectionInfo &AddedSection : Config.AddSection) {
Expand All @@ -924,15 +925,15 @@ static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
return Error::success();
};
if (Error E = handleUserSection(AddedSection, AddSection))
return E;
return createFileError(Config.InputFilename, std::move(E));
}

for (const NewSectionInfo &NewSection : Config.UpdateSection) {
auto UpdateSection = [&](StringRef Name, ArrayRef<uint8_t> Data) {
return Obj.updateSection(Name, Data);
};
if (Error E = handleUserSection(NewSection, UpdateSection))
return E;
return createFileError(Config.InputFilename, std::move(E));
}

if (!Config.AddGnuDebugLink.empty())
Expand All @@ -943,7 +944,7 @@ static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
// before adding new symbols.
if (!Obj.SymbolTable && !Config.SymbolsToAdd.empty())
if (Error E = Obj.addNewSymbolTable())
return E;
return createFileError(Config.InputFilename, std::move(E));

for (const NewSymbolInfo &SI : Config.SymbolsToAdd)
addSymbol(Obj, SI, ELFConfig.NewSymbolVisibility);
Expand All @@ -955,7 +956,7 @@ static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
if (Iter != Config.SetSectionFlags.end()) {
const SectionFlagsUpdate &SFU = Iter->second;
if (Error E = setSectionFlagsAndType(Sec, SFU.NewFlags, Obj.Machine))
return E;
return createFileError(Config.InputFilename, std::move(E));
}
auto It2 = Config.SetSectionType.find(Sec.Name);
if (It2 != Config.SetSectionType.end())
Expand All @@ -974,7 +975,7 @@ static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
Sec.Name = std::string(SR.NewName);
if (SR.NewFlags) {
if (Error E = setSectionFlagsAndType(Sec, *SR.NewFlags, Obj.Machine))
return E;
return createFileError(Config.InputFilename, std::move(E));
}
RenamedSections.insert(&Sec);
} else if (RelocSec && !(Sec.Flags & SHF_ALLOC))
Expand Down Expand Up @@ -1091,7 +1092,7 @@ Error objcopy::elf::executeObjcopyOnBinary(const CommonConfig &Config,
: getOutputElfType(In);

if (Error E = handleArgs(Config, ELFConfig, OutputElfType, **Obj))
return createFileError(Config.InputFilename, std::move(E));
return E;

if (Error E = writeOutput(Config, **Obj, Out, OutputElfType))
return createFileError(Config.InputFilename, std::move(E));
Expand Down
27 changes: 14 additions & 13 deletions llvm/lib/ObjCopy/MachO/MachOObjcopy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -306,25 +306,25 @@ static Error processLoadCommands(const MachOConfig &MachOConfig, Object &Obj) {
}

static Error dumpSectionToFile(StringRef SecName, StringRef Filename,
Object &Obj) {
StringRef InputFilename, Object &Obj) {
for (LoadCommand &LC : Obj.LoadCommands)
for (const std::unique_ptr<Section> &Sec : LC.Sections) {
if (Sec->CanonicalName == SecName) {
Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
FileOutputBuffer::create(Filename, Sec->Content.size());
if (!BufferOrErr)
return BufferOrErr.takeError();
return createFileError(Filename, BufferOrErr.takeError());
std::unique_ptr<FileOutputBuffer> Buf = std::move(*BufferOrErr);
llvm::copy(Sec->Content, Buf->getBufferStart());

if (Error E = Buf->commit())
return E;
return createFileError(Filename, std::move(E));
return Error::success();
}
}

return createStringError(object_error::parse_failed, "section '%s' not found",
SecName.str().c_str());
return createFileError(InputFilename, object_error::parse_failed,
"section '%s' not found", SecName.str().c_str());
}

static Error addSection(const NewSectionInfo &NewSection, Object &Obj) {
Expand Down Expand Up @@ -426,12 +426,13 @@ static Error handleArgs(const CommonConfig &Config,
StringRef SectionName;
StringRef FileName;
std::tie(SectionName, FileName) = Flag.split('=');
if (Error E = dumpSectionToFile(SectionName, FileName, Obj))
if (Error E =
dumpSectionToFile(SectionName, FileName, Config.InputFilename, Obj))
return E;
}

if (Error E = removeSections(Config, Obj))
return E;
return createFileError(Config.InputFilename, std::move(E));

// Mark symbols to determine which symbols are still needed.
if (Config.StripAll)
Expand All @@ -446,20 +447,20 @@ static Error handleArgs(const CommonConfig &Config,

for (const NewSectionInfo &NewSection : Config.AddSection) {
if (Error E = isValidMachOCannonicalName(NewSection.SectionName))
return E;
return createFileError(Config.InputFilename, std::move(E));
if (Error E = addSection(NewSection, Obj))
return E;
return createFileError(Config.InputFilename, std::move(E));
}

for (const NewSectionInfo &NewSection : Config.UpdateSection) {
if (Error E = isValidMachOCannonicalName(NewSection.SectionName))
return E;
return createFileError(Config.InputFilename, std::move(E));
if (Error E = updateSection(NewSection, Obj))
return E;
return createFileError(Config.InputFilename, std::move(E));
}

if (Error E = processLoadCommands(MachOConfig, Obj))
return E;
return createFileError(Config.InputFilename, std::move(E));

return Error::success();
}
Expand All @@ -479,7 +480,7 @@ Error objcopy::macho::executeObjcopyOnBinary(const CommonConfig &Config,
Config.InputFilename.str().c_str());

if (Error E = handleArgs(Config, MachOConfig, **O))
return createFileError(Config.InputFilename, std::move(E));
return E;

// Page size used for alignment of segment sizes in Mach-O executables and
// dynamic libraries.
Expand Down
15 changes: 8 additions & 7 deletions llvm/lib/ObjCopy/wasm/WasmObjcopy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,23 +38,23 @@ static bool isCommentSection(const Section &Sec) {
}

static Error dumpSectionToFile(StringRef SecName, StringRef Filename,
Object &Obj) {
StringRef InputFilename, Object &Obj) {
for (const Section &Sec : Obj.Sections) {
if (Sec.Name == SecName) {
ArrayRef<uint8_t> Contents = Sec.Contents;
Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
FileOutputBuffer::create(Filename, Contents.size());
if (!BufferOrErr)
return BufferOrErr.takeError();
return createFileError(Filename, BufferOrErr.takeError());
std::unique_ptr<FileOutputBuffer> Buf = std::move(*BufferOrErr);
std::copy(Contents.begin(), Contents.end(), Buf->getBufferStart());
if (Error E = Buf->commit())
return E;
return createFileError(Filename, std::move(E));
return Error::success();
}
}
return createStringError(errc::invalid_argument, "section '%s' not found",
SecName.str().c_str());
return createFileError(Filename, errc::invalid_argument,
"section '%s' not found", SecName.str().c_str());
}

static void removeSections(const CommonConfig &Config, Object &Obj) {
Expand Down Expand Up @@ -115,8 +115,9 @@ static Error handleArgs(const CommonConfig &Config, Object &Obj) {
StringRef SecName;
StringRef FileName;
std::tie(SecName, FileName) = Flag.split("=");
if (Error E = dumpSectionToFile(SecName, FileName, Obj))
return createFileError(FileName, std::move(E));
if (Error E =
dumpSectionToFile(SecName, FileName, Config.InputFilename, Obj))
return E;
}

removeSections(Config, Obj);
Expand Down
4 changes: 4 additions & 0 deletions llvm/test/tools/llvm-objcopy/ELF/dump-section.test
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,7 @@ ProgramHeaders:
# RUN: not llvm-objcopy --dump-section .text= %t /dev/null 2>&1 | FileCheck %s --check-prefix=ERR2

# ERR2: error: bad format for --dump-section, expected section=file

# RUN: not llvm-objcopy --dump-section .text=not_exists/text-section %t 2>&1 \
# RUN: | FileCheck -DMSG=%errc_ENOENT %s -DINPUT=%t --check-prefix=NO-SUCH-PATH
# NO-SUCH-PATH: error: 'not_exists/text-section': [[MSG]]
4 changes: 4 additions & 0 deletions llvm/test/tools/llvm-objcopy/MachO/dump-section.test
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@
# RUN: | FileCheck %s -DINPUT=%t --check-prefix=NO-SUCH-SECTION
# NO-SUCH-SECTION: error: '[[INPUT]]': section '__TEXT,__foo' not found

# RUN: not llvm-objcopy --dump-section __TEXT,__text=not_exists/text-section %t 2>&1 \
# RUN: | FileCheck -DMSG=%errc_ENOENT %s -DINPUT=%t --check-prefix=NO-SUCH-PATH
# NO-SUCH-PATH: error: 'not_exists/text-section': [[MSG]]

--- !mach-o
FileHeader:
magic: 0xFEEDFACF
Expand Down
4 changes: 4 additions & 0 deletions llvm/test/tools/llvm-objcopy/wasm/dump-section.test
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@

# REMOVED-NOT: producers

# RUN: not llvm-objcopy --dump-section producers=not_exists/text-section %t 2>&1 \
# RUN: | FileCheck -DMSG=%errc_ENOENT %s -DINPUT=%t --check-prefix=NO-SUCH-PATH
# NO-SUCH-PATH: error: 'not_exists/text-section': [[MSG]]

--- !WASM
FileHeader:
Version: 0x00000001
Expand Down