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

AmrDeveloper · 2025-02-01T11:48:33Z

Fix printing the correct file path in the error message when the output file specified by --dump-section cannot be opened

Fixes: #125113 on ELF, MachO, Wasm

llvmbot · 2025-02-01T11:49:09Z

@llvm/pr-subscribers-llvm-support
@llvm/pr-subscribers-backend-webassembly

@llvm/pr-subscribers-llvm-binary-utilities

Author: Amr Hesham (AmrDeveloper)

Changes

Change the way of creating new file errors to be aware of which file it should report to

Fixes: #125113 on ELF, MachO, Wasm

Full diff: https://github.com/llvm/llvm-project/pull/125345.diff

6 Files Affected:

(modified) llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp (+40-26)
(modified) llvm/lib/ObjCopy/MachO/MachOObjcopy.cpp (+15-13)
(modified) llvm/lib/ObjCopy/wasm/WasmObjcopy.cpp (+9-7)
(added) llvm/test/tools/llvm-objcopy/ELF/dump-section-directory-not-exists.test (+36)
(added) llvm/test/tools/llvm-objcopy/MachO/dump-section-directory-not-exists.test (+67)
(added) llvm/test/tools/llvm-objcopy/wasm/dump-section-directory-not-exists.test (+24)

diff --git a/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp b/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
index 5aa0079f3fbc7a7..9d979a7c094d415 100644
--- a/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
+++ b/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
@@ -186,27 +186,32 @@ 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());
+      if (Sec.Type == SHT_NOBITS) {
+        Error E =
+            createStringError(object_error::parse_failed,
+                              "cannot dump section '%s': it has no contents",
+                              SecName.str().c_str());
+        return createFileError(InputFilename, std::move(E));
+      }
       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());
+  Error E = createStringError(object_error::parse_failed,
+                              "section '%s' not found", SecName.str().c_str());
+
+  return createFileError(InputFilename, std::move(E));
 }
 
 Error Object::compressOrDecompressSections(const CommonConfig &Config) {
@@ -798,7 +803,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;
   }
 
@@ -807,10 +813,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()) {
@@ -826,21 +832,24 @@ 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(
+          Error E = createStringError(
               errc::invalid_argument,
               "address 0x" + Twine::utohexstr(Seg.PAddr) +
                   " cannot be increased by 0x" +
                   Twine::utohexstr(Config.ChangeSectionLMAValAll) +
                   ". The result would overflow");
+          return createFileError(Config.InputFilename, std::move(E));
         } else if (Config.ChangeSectionLMAValAll < 0 &&
                    Seg.PAddr < std::numeric_limits<uint64_t>::min() -
                                    Config.ChangeSectionLMAValAll) {
-          return createStringError(
+          Error E = createStringError(
               errc::invalid_argument,
               "address 0x" + Twine::utohexstr(Seg.PAddr) +
                   " cannot be decreased by 0x" +
                   Twine::utohexstr(std::abs(Config.ChangeSectionLMAValAll)) +
                   ". The result would underflow");
+
+          return createFileError(Config.InputFilename, std::move(E));
         }
         Seg.PAddr += Config.ChangeSectionLMAValAll;
       }
@@ -848,11 +857,12 @@ static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
   }
 
   if (!Config.ChangeSectionAddress.empty()) {
-    if (Obj.Type != ELF::ET_REL)
-      return createStringError(
+    if (Obj.Type != ELF::ET_REL) {
+      Error E = createStringError(
           object_error::invalid_file_type,
           "cannot change section address in a non-relocatable file");
-
+      return createFileError(Config.InputFilename, std::move(E));
+    }
     StringMap<AddressUpdate> SectionsToUpdateAddress;
     for (const SectionPatternAddressUpdate &PatternUpdate :
          make_range(Config.ChangeSectionAddress.rbegin(),
@@ -863,22 +873,26 @@ static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
                 .second) {
           if (PatternUpdate.Update.Kind == AdjustKind::Subtract &&
               Sec.Addr < PatternUpdate.Update.Value) {
-            return createStringError(
+            Error E = createStringError(
                 errc::invalid_argument,
                 "address 0x" + Twine::utohexstr(Sec.Addr) +
                     " cannot be decreased by 0x" +
                     Twine::utohexstr(PatternUpdate.Update.Value) +
                     ". The result would underflow");
+
+            return createFileError(Config.InputFilename, std::move(E));
           }
           if (PatternUpdate.Update.Kind == AdjustKind::Add &&
               Sec.Addr > std::numeric_limits<uint64_t>::max() -
                              PatternUpdate.Update.Value) {
-            return createStringError(
+            Error E = createStringError(
                 errc::invalid_argument,
                 "address 0x" + Twine::utohexstr(Sec.Addr) +
                     " cannot be increased by 0x" +
                     Twine::utohexstr(PatternUpdate.Update.Value) +
                     ". The result would overflow");
+
+            return createFileError(Config.InputFilename, std::move(E));
           }
 
           switch (PatternUpdate.Update.Kind) {
@@ -909,7 +923,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) {
@@ -924,7 +938,7 @@ 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) {
@@ -932,7 +946,7 @@ static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
       return Obj.updateSection(Name, Data);
     };
     if (Error E = handleUserSection(NewSection, UpdateSection))
-      return E;
+      return createFileError(Config.InputFilename, std::move(E));
   }
 
   if (!Config.AddGnuDebugLink.empty())
@@ -943,7 +957,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);
@@ -955,7 +969,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())
@@ -974,7 +988,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))
@@ -1091,7 +1105,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));
diff --git a/llvm/lib/ObjCopy/MachO/MachOObjcopy.cpp b/llvm/lib/ObjCopy/MachO/MachOObjcopy.cpp
index a188425b283fa63..49ac903e9957326 100644
--- a/llvm/lib/ObjCopy/MachO/MachOObjcopy.cpp
+++ b/llvm/lib/ObjCopy/MachO/MachOObjcopy.cpp
@@ -306,25 +306,26 @@ 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());
+  Error E = createStringError(object_error::parse_failed,
+                              "section '%s' not found", SecName.str().c_str());
+  return createFileError(InputFilename, std::move(E));
 }
 
 static Error addSection(const NewSectionInfo &NewSection, Object &Obj) {
@@ -426,12 +427,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)
@@ -446,20 +448,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();
 }
@@ -479,7 +481,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.
diff --git a/llvm/lib/ObjCopy/wasm/WasmObjcopy.cpp b/llvm/lib/ObjCopy/wasm/WasmObjcopy.cpp
index cf3d884bee3bd8e..3ace39ac558f5b4 100644
--- a/llvm/lib/ObjCopy/wasm/WasmObjcopy.cpp
+++ b/llvm/lib/ObjCopy/wasm/WasmObjcopy.cpp
@@ -38,23 +38,24 @@ 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());
+  Error E = createStringError(errc::invalid_argument, "section '%s' not found",
+                              SecName.str().c_str());
+  return createFileError(Filename, std::move(E));
 }
 
 static void removeSections(const CommonConfig &Config, Object &Obj) {
@@ -115,8 +116,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);
diff --git a/llvm/test/tools/llvm-objcopy/ELF/dump-section-directory-not-exists.test b/llvm/test/tools/llvm-objcopy/ELF/dump-section-directory-not-exists.test
new file mode 100644
index 000000000000000..462ebd691ae49ef
--- /dev/null
+++ b/llvm/test/tools/llvm-objcopy/ELF/dump-section-directory-not-exists.test
@@ -0,0 +1,36 @@
+## Show that llvm-objcopy report that section file path is not exists.
+
+# RUN: yaml2obj %s -o %t
+
+# RUN: not llvm-objcopy --dump-section .text=not_exists/text-section %t 2>&1 \
+# RUN:   | FileCheck %s -DINPUT=%t --check-prefix=NO-SUCH-PATH
+# NO-SUCH-PATH: error: 'not_exists/text-section': No such file or directory
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000001000
+    Content:         "DEADBEEF"
+  - Name:            .foo
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_WRITE ]
+    Content:         "CAFE"
+  - Name:            .empty
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+  - Name:            .bar
+    Type:            SHT_NOBITS
+    Flags:           [ SHF_WRITE ]
+ProgramHeaders:
+  - Type:     PT_LOAD
+    Flags:    [ PF_X, PF_R ]
+    FirstSec: .text
+    LastSec:  .text
+
diff --git a/llvm/test/tools/llvm-objcopy/MachO/dump-section-directory-not-exists.test b/llvm/test/tools/llvm-objcopy/MachO/dump-section-directory-not-exists.test
new file mode 100644
index 000000000000000..ac597b362b4c7b1
--- /dev/null
+++ b/llvm/test/tools/llvm-objcopy/MachO/dump-section-directory-not-exists.test
@@ -0,0 +1,67 @@
+## Show that llvm-objcopy report that section file path is not exists.
+
+# RUN: yaml2obj %s -o %t
+
+# RUN: not llvm-objcopy --dump-section __TEXT,__text=not_exists/text-section %t 2>&1 \
+# RUN:   | FileCheck %s -DINPUT=%t --check-prefix=NO-SUCH-PATH
+# NO-SUCH-PATH: error: 'not_exists/text-section': No such file or directory
+
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x01000007
+  cpusubtype:      0x00000003
+  filetype:        0x00000001
+  ncmds:           1
+  sizeofcmds:      312
+  flags:           0x00002000
+  reserved:        0x00000000
+LoadCommands:
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         312
+    segname:         ''
+    vmaddr:          0
+    vmsize:          12
+    fileoff:         344
+    filesize:        12
+    maxprot:         7
+    initprot:        7
+    nsects:          3
+    flags:           0
+    Sections:
+      - sectname:        __text
+        segname:         __TEXT
+        addr:            0x0000000000000000
+        content:         'AABBCCDD'
+        size:            4
+        offset:          344
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x80000400
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+      - sectname:        __data
+        segname:         __DATA
+        addr:            0x0000000000000004
+        content:         'EEFFEEFF'
+        size:            4
+        offset:          348
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x00000000
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+      - sectname:        __const
+        segname:         __TEXT
+        addr:            0x0000000000000008
+        content:         'EEFFEEFF'
+        size:            4
+        offset:          352
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x00000000
+        reserved1:       0x00000000
+        reserved2:       0x00000000
diff --git a/llvm/test/tools/llvm-objcopy/wasm/dump-section-directory-not-exists.test b/llvm/test/tools/llvm-objcopy/wasm/dump-section-directory-not-exists.test
new file mode 100644
index 000000000000000..f0ac8e980cf5dcf
--- /dev/null
+++ b/llvm/test/tools/llvm-objcopy/wasm/dump-section-directory-not-exists.test
@@ -0,0 +1,24 @@
+## Show that llvm-objcopy report that section file path is not exists.
+
+# RUN: yaml2obj %s -o %t
+
+# RUN: not llvm-objcopy --dump-section producers=not_exists/text-section %t 2>&1 \
+# RUN:   | FileCheck %s -DINPUT=%t --check-prefix=NO-SUCH-PATH
+# NO-SUCH-PATH: error: 'not_exists/text-section': No such file or directory
+
+--- !WASM
+FileHeader:
+  Version: 0x00000001
+Sections:
+  - Type: TYPE
+    Signatures:
+      - Index: 0
+        ParamTypes:
+          - I32
+        ReturnTypes:
+          - F32
+  - Type: CUSTOM
+    Name: producers
+    Tools:
+      - Name:   clang
+        Version: 9.0.0

MaskRay · 2025-02-01T20:58:29Z

llvm/test/tools/llvm-objcopy/ELF/dump-section-directory-not-exists.test

+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000001000
+    Content:         "DEADBEEF"
+  - Name:            .foo


Keep just .text and delete unused sections.

MaskRay · 2025-02-01T20:59:11Z

llvm/test/tools/llvm-objcopy/ELF/dump-section-directory-not-exists.test

+# RUN: not llvm-objcopy --dump-section .text=not_exists/text-section %t 2>&1 \
+# RUN:   | FileCheck %s -DINPUT=%t --check-prefix=NO-SUCH-PATH
+# Don't check the OS-dependent message "No such file or directory".
+# NO-SUCH-PATH: error: 'not_exists/text-section':


Can use -DMSG=%errc_ENOENT. See examples in the ELF/ directory

MaskRay · 2025-02-01T20:59:44Z

llvm/test/tools/llvm-objcopy/ELF/dump-section-directory-not-exists.test

+  - Name:            .bar
+    Type:            SHT_NOBITS
+    Flags:           [ SHF_WRITE ]
+ProgramHeaders:


Delete unused ProgramHeaders and trailing blank lines

MaskRay · 2025-02-01T21:01:42Z

llvm/test/tools/llvm-objcopy/ELF/dump-section-directory-not-exists.test

@@ -0,0 +1,37 @@
+## Show that llvm-objcopy report that section file path is not exists.


Place the test into the primary test dump-section.test. We often use one test file for both positive and negative tests. Sometimes this actually improves test discoverability

All comments addressed, thank you

llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp

llvm/test/tools/llvm-objcopy/ELF/dump-section.test

AmrDeveloper · 2025-02-04T17:48:18Z

Please take a final look when you have time @MaskRay @jh7370

jh7370 · 2025-02-05T09:17:17Z

Please take a final look when you have time @MaskRay @jh7370

Hi @AmrDeveloper,

The usual policy is to wait a week between an update and pinging for another review. Reviewers get email notifications when you update anyway, so we're typically aware that an updated review is needed, but for me at least, I only have time to do a limited amount of LLVM reviewing on a given day.

Also, per LLVM policy, please don't force push when making an update to the PR:

When updating a pull request, you should push additional “fix up” commits to your branch instead of force pushing. This makes it easier for GitHub to track the context of previous review comments. Consider using the built-in support for fixups in git.

Force pushes make it significantly harder to track what's changed between two versions of a patch and they're unnecessary - you can do fixup commits to address issues and if you need to rebase on main, you can generally use a merge commit from main (any merge commits and fixups will disappear when the PR is squashed and merged at the end).

I'll take a look at this PR again now.

jh7370 · 2025-02-05T09:18:39Z

llvm/include/llvm/Support/Error.h

@@ -1404,6 +1404,22 @@ inline Error createFileError(const Twine &F, size_t Line, std::error_code EC) {
  return createFileError(F, Line, errorCodeToError(EC));
 }

+/// Concatenate a source file path with a std::error_code and string error
+inline Error createFileError(const Twine &F, std::error_code EC,


If you want to add this helper to the Error.h header, you should do it in a separate, prerequisite PR and include unit tests.

jh7370 · 2025-02-05T09:24:03Z

llvm/include/llvm/Support/Error.h

@@ -1404,6 +1404,22 @@ inline Error createFileError(const Twine &F, size_t Line, std::error_code EC) {
  return createFileError(F, Line, errorCodeToError(EC));
 }

+/// Concatenate a source file path with a std::error_code and string error


Nit: Comments in LLVM require a trailing full stop ('.').

I also don't think this and the equivalent comment below make a huge amount of sense: you're not concatenating the file path with the std::error_code at all. Better would be to base the comment on the createStringError comments "Create a StringError with the specified error code and prepend the file path to it."

AmrDeveloper · 2025-02-05T17:43:14Z

The usual policy is to wait a week between an update and pinging for another review. Reviewers get email notifications when you update anyway

Sorry for that, I will read policy again :D

Force pushes make it significantly harder to track what's changed between two versions of a patch and they're unnecessary - you can do fixup commits to address issues and if you need to rebase on main, you can generally use a merge commit from main (any merge commits and fixups will disappear when the PR is squashed and merged at the end).

Thanks for informing me,

I will create another PR for Error functions with unit tests and update docs, then we can continue with this PR after merging the other one

Thank you

Add new CreateFileError functions to create a StringError with the specified error code and prepend the file path to it Needed for: #125345

… exist

AmrDeveloper · 2025-02-06T17:40:05Z

@jh7370 This force push is because I modified the old comment to fix the conflict by rebase not merge 😅

Also I added a release note :D

MaskRay · 2025-02-07T06:37:28Z

The description (which will be used as the default final commit message) should mention --dump-section .

MaskRay · 2025-02-07T06:38:37Z

llvm/docs/ReleaseNotes.md

@@ -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.


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

jh7370

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 think this is worth cherry-picking to the release branch, @AmrDeveloper. What do you think? Instructions are here for once the PR is merged: https://llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches

jh7370 · 2025-02-07T06:33:19Z

llvm/docs/ReleaseNotes.md

@@ -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.


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.

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."

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

"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

MaskRay · 2025-02-08T05:40:22Z

llvm/docs/ReleaseNotes.md

@@ -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.


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.

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

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

AmrDeveloper · 2025-02-08T13:19:30Z

/cherry-pick 66bea0d

llvmbot · 2025-02-08T13:20:52Z

/pull-request #126367

AmrDeveloper · 2025-02-10T18:19:00Z

/cherry-pick 66bea0d 2464f4b

…esn't exist (llvm#125345) Fix printing the correct file path in the error message when the output file specified by `--dump-section` cannot be opened Fixes: llvm#125113 on ELF, MachO, Wasm (cherry picked from commit 66bea0d)

Add new CreateFileError functions to create a StringError with the specified error code and prepend the file path to it Needed for: llvm#125345 (cherry picked from commit 2464f4b)

… dump-section output path doesn't exist llvm#125345

… dump-section output path doesn't exist #125345 (#126607) Add release note for llvm-objcopy fixing prints wrong path when dump-section output path doesn't exist in #125345

Add new CreateFileError functions to create a StringError with the specified error code and prepend the file path to it Needed for: llvm#125345

…esn't exist (llvm#125345) Fix printing the correct file path in the error message when the output file specified by `--dump-section` cannot be opened Fixes: llvm#125113 on ELF, MachO, Wasm

AmrDeveloper added the tools:llvm-objcopy/strip label Feb 1, 2025

llvmbot added backend:WebAssembly llvm:binary-utilities labels Feb 1, 2025

AmrDeveloper force-pushed the fix_objcopy_wrong_path_diagnostic branch 2 times, most recently from cf63b7e to 7fb033f Compare February 1, 2025 17:00

AmrDeveloper requested review from alexander-shaposhnikov and MaskRay February 1, 2025 20:33

MaskRay reviewed Feb 1, 2025

View reviewed changes

AmrDeveloper requested a review from MaskRay February 1, 2025 22:51

jh7370 reviewed Feb 3, 2025

View reviewed changes

llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp Outdated Show resolved Hide resolved

llvm/test/tools/llvm-objcopy/ELF/dump-section.test Outdated Show resolved Hide resolved

llvmbot added the llvm:support label Feb 3, 2025

AmrDeveloper requested a review from jh7370 February 3, 2025 18:08

AmrDeveloper force-pushed the fix_objcopy_wrong_path_diagnostic branch from 5f9c408 to 6f2a030 Compare February 3, 2025 18:46

jh7370 reviewed Feb 5, 2025

View reviewed changes

AmrDeveloper mentioned this pull request Feb 5, 2025

[LLVM][Support] Add new CreateFileError functions #125906

Merged

AmrDeveloper added a commit that referenced this pull request Feb 6, 2025

[LLVM][Support] Add new CreateFileError functions (#125906)

2464f4b

Add new CreateFileError functions to create a StringError with the specified error code and prepend the file path to it Needed for: #125345

AmrDeveloper added 6 commits February 6, 2025 18:21

[llvm-objcopy] Fix prints wrong path when section output path doesn't…

d80b0ee

… exist

Don't check the OS-dependent message

02e66cb

Address code review comments

9a8f4c6

Remove extra new lines

830a973

Address code reviews

df9101e

Add releasenote

a6be4bd

AmrDeveloper force-pushed the fix_objcopy_wrong_path_diagnostic branch from 6f2a030 to a6be4bd Compare February 6, 2025 17:36

MaskRay reviewed Feb 7, 2025

View reviewed changes

jh7370 approved these changes Feb 7, 2025

View reviewed changes

AmrDeveloper added 2 commits February 7, 2025 19:12

Update releasenote

37289e6

Merge branch 'llvm:main' into fix_objcopy_wrong_path_diagnostic

48c1578

MaskRay approved these changes Feb 8, 2025

View reviewed changes

Remove release note from this PR

0c3c39e

AmrDeveloper changed the title ~~[llvm-objcopy] Fix prints wrong path when section output path doesn't exist~~ [llvm-objcopy] Fix prints wrong path when dump-section output path doesn't exist Feb 8, 2025

AmrDeveloper merged commit 66bea0d into llvm:main Feb 8, 2025
8 checks passed

AmrDeveloper added this to the LLVM 20.X Release milestone Feb 8, 2025

AmrDeveloper added a commit to AmrDeveloper/llvm-project that referenced this pull request Feb 10, 2025

release/20.x: [llvm-objcopy][ReleaseNotes] Fix prints wrong path when…

8886b33

… dump-section output path doesn't exist llvm#125345

AmrDeveloper mentioned this pull request Feb 10, 2025

release/20.x: [llvm-objcopy][ReleaseNotes] Fix prints wrong path when dump-section output path doesn't exist #125345 #126607

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AmrDeveloper commented Feb 1, 2025 •

edited

Loading

llvmbot commented Feb 1, 2025 •

edited

Loading

MaskRay Feb 1, 2025

MaskRay Feb 1, 2025

MaskRay Feb 1, 2025

MaskRay Feb 1, 2025

AmrDeveloper Feb 1, 2025

AmrDeveloper commented Feb 4, 2025

jh7370 commented Feb 5, 2025

jh7370 Feb 5, 2025

jh7370 Feb 5, 2025

AmrDeveloper commented Feb 5, 2025

AmrDeveloper commented Feb 6, 2025 •

edited

Loading

MaskRay commented Feb 7, 2025

MaskRay Feb 7, 2025

jh7370 left a comment

jh7370 Feb 7, 2025

jh7370 Feb 7, 2025

AmrDeveloper Feb 7, 2025

AmrDeveloper Feb 7, 2025

MaskRay Feb 8, 2025

AmrDeveloper Feb 8, 2025

AmrDeveloper Feb 8, 2025

AmrDeveloper commented Feb 8, 2025

llvmbot commented Feb 8, 2025

AmrDeveloper commented Feb 10, 2025

		@@ -0,0 +1,37 @@
		## Show that llvm-objcopy report that section file path is not exists.

	- 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.

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

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

Conversation

AmrDeveloper commented Feb 1, 2025 • edited Loading

llvmbot commented Feb 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmrDeveloper commented Feb 4, 2025

jh7370 commented Feb 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmrDeveloper commented Feb 5, 2025

AmrDeveloper commented Feb 6, 2025 • edited Loading

MaskRay commented Feb 7, 2025

Choose a reason for hiding this comment

jh7370 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmrDeveloper commented Feb 8, 2025

llvmbot commented Feb 8, 2025

AmrDeveloper commented Feb 10, 2025

AmrDeveloper commented Feb 1, 2025 •

edited

Loading

llvmbot commented Feb 1, 2025 •

edited

Loading

AmrDeveloper commented Feb 6, 2025 •

edited

Loading