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.

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

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

Merged
merged 9 commits into from
Feb 8, 2025
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d80b0ee
[llvm-objcopy] Fix prints wrong path when section output path doesn't…
AmrDeveloper Feb 1, 2025
02e66cb
Don't check the OS-dependent message
AmrDeveloper Feb 1, 2025
9a8f4c6
Address code review comments
AmrDeveloper Feb 1, 2025
830a973
Remove extra new lines
AmrDeveloper Feb 3, 2025
df9101e
Address code reviews
AmrDeveloper Feb 3, 2025
a6be4bd
Add releasenote
AmrDeveloper Feb 6, 2025
37289e6
Update releasenote
AmrDeveloper Feb 7, 2025
48c1578
Merge branch 'llvm:main' into fix_objcopy_wrong_path_diagnostic
AmrDeveloper Feb 7, 2025
0c3c39e
Remove release note from this PR
AmrDeveloper Feb 8, 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
3 changes: 3 additions & 0 deletions llvm/docs/ReleaseNotes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,9 @@ Changes to the Debug Info
Changes to the LLVM tools
---------------------------------

- 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

Choose a reason for hiding this comment

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

IIRC the wrapped line should be indented in markdown

If this is going to be merged into release/20.x, then this could be removed to avoid a merge conflict.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, I will remove it from this PR and merge after CI become green, then open pr to put it on release

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the title and message, i will merge and run cherry pick command


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

Expand Down
59 changes: 30 additions & 29 deletions llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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