Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
16 changes: 8 additions & 8 deletions llvm/lib/ObjCopy/ConfigManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -122,14 +122,14 @@ ConfigManager::getDXContainerConfig() const {
if (!Common.AddGnuDebugLink.empty() || !Common.SplitDWO.empty() ||
!Common.AllocSectionsPrefix.empty() ||
Common.DiscardMode != DiscardType::None || !Common.AddSection.empty() ||
!Common.DumpSection.empty() || !Common.KeepSection.empty() ||
!Common.SectionsToRename.empty() || !Common.SetSectionAlignment.empty() ||
!Common.SetSectionFlags.empty() || !Common.SetSectionType.empty() ||
Common.ExtractDWO || Common.OnlyKeepDebug || Common.StripAllGNU ||
Common.StripDWO || Common.StripDebug || Common.StripNonAlloc ||
Common.StripSections || Common.StripUnneeded ||
Common.DecompressDebugSections || Common.GapFill != 0 ||
Common.PadTo != 0 || Common.ChangeSectionLMAValAll != 0 ||
!Common.KeepSection.empty() || !Common.SectionsToRename.empty() ||
!Common.SetSectionAlignment.empty() || !Common.SetSectionFlags.empty() ||
!Common.SetSectionType.empty() || Common.ExtractDWO ||
Common.OnlyKeepDebug || Common.StripAllGNU || Common.StripDWO ||
Common.StripDebug || Common.StripNonAlloc || Common.StripSections ||
Common.StripUnneeded || Common.DecompressDebugSections ||
Common.GapFill != 0 || Common.PadTo != 0 ||
Common.ChangeSectionLMAValAll != 0 ||
!Common.ChangeSectionAddress.empty()) {
return createStringError(llvm::errc::invalid_argument,
"option is not supported for DXContainer");
Expand Down
44 changes: 44 additions & 0 deletions llvm/lib/ObjCopy/DXContainer/DXContainerObjcopy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@
#include "llvm/ObjCopy/DXContainer/DXContainerObjcopy.h"
#include "DXContainerReader.h"
#include "DXContainerWriter.h"
#include "llvm/BinaryFormat/DXContainer.h"
#include "llvm/ObjCopy/CommonConfig.h"
#include "llvm/ObjCopy/DXContainer/DXContainerConfig.h"
#include "llvm/Support/FileOutputBuffer.h"
#include "llvm/Support/raw_ostream.h"

namespace llvm {
Expand Down Expand Up @@ -42,7 +44,49 @@ static Error extractPartAsObject(StringRef PartName, StringRef OutFilename,
"part '%s' not found", PartName.str().c_str());
}

static Error dumpPartToFile(StringRef PartName, StringRef Filename,
StringRef InputFilename, Object &Obj) {
for (const Part &P : Obj.Parts) {
if (P.Name == PartName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a weird pattern for this. Why not use find_if and return early if it isn't found instead?

  auto PartIter = llvm::find_if(
      Obj.Parts, [&PartName](const Part &P) { return P.Name == PartName; });
  if (PartIter == Obj.Parts.end())
    return createFileError(...);

ArrayRef<uint8_t> Contents = P.Data;
// The DXContainer format is a bit odd because the part-specific headers
// are contained inside the part data itself. For parts that contain LLVM
// bitcode when we dump the part we want to skip the part-specific header
// so that we get a valid .bc file that we can inspect. All the data
// contained inside the program header is pulled out of the bitcode, so
// the header can be reconstructed if needed from the bitcode itself.
// More comprehensive documentation on the DXContainer format can be found
// at https://llvm.org/docs/DirectX/DXContainer.html.
if (PartName == "DXIL" || PartName == "STAT")
Contents = Contents.drop_front(sizeof(llvm::dxbc::ProgramHeader));
if (Contents.empty())
return createFileError(Filename, object_error::parse_failed,
"part '%s' is empty", PartName.str().c_str());
Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
FileOutputBuffer::create(Filename, Contents.size());
if (!BufferOrErr)
return createFileError(Filename, BufferOrErr.takeError());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Test cases for this and other error paths are required.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume it's not practical to test both the create and commit error paths? (I see one is tested, but not the other).

std::unique_ptr<FileOutputBuffer> Buf = std::move(*BufferOrErr);
llvm::copy(Contents, Buf->getBufferStart());
if (Error E = Buf->commit())
return createFileError(Filename, std::move(E));
Copy link
Contributor

Choose a reason for hiding this comment

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

ELF behaviour is to create an error if the contents are empty. I am not sure if that is generated here? Can we add a test-case to demonstrate that error is generated

return Error::success();
}
}
return createFileError(Filename,
std::make_error_code(std::errc::invalid_argument),
"part '%s' not found", PartName.str().c_str());
}

static Error handleArgs(const CommonConfig &Config, Object &Obj) {
for (StringRef Flag : Config.DumpSection) {
StringRef SecName;
StringRef FileName;
std::tie(SecName, FileName) = Flag.split("=");
if (Error E = dumpPartToFile(SecName, FileName, Config.InputFilename, Obj))
return E;
}

// Extract all sections before any modifications.
for (StringRef Flag : Config.ExtractSection) {
StringRef SectionName;
Expand Down
27 changes: 27 additions & 0 deletions llvm/test/tools/llvm-objcopy/DXContainer/dump-section-errors.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# RUN: yaml2obj %s -o %t.dxbc
# RUN: not llvm-objcopy --dump-section=FKE0=%t.fek0 %t.dxbc 2>&1 | FileCheck %s --check-prefix=CHECK-EMPTY -DFILE=%t.fek0
Copy link
Contributor

Choose a reason for hiding this comment

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

FileCheck has a CHECK-EMPTY directive so this prefix could be confusing / ambiguous. Might be worth a different name to avoid that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As the one who implemented CHECK-EMPTY, I should have seen that 😂

# RUN: not llvm-objcopy --dump-section=FKE3=%t.fek1 %t.dxbc 2>&1 | FileCheck %s --check-prefix=CHECK-MISSING -DFILE=%t.fek1
# RUN: not llvm-objcopy --dump-section=FKE2=%t/does_not_exist/.fek2 %t.dxbc 2>&1 | FileCheck %s --check-prefix=CHECK-BAD-PATH -DFILE=%t/does_not_exist/.fek2 -DMSG=%errc_ENOENT

# CHECK-EMPTY: error: '[[FILE]]': part 'FKE0' is empty
# CHECK-MISSING: error: '[[FILE]]': part 'FKE3' not found
# CHECK-BAD-PATH: error: '[[FILE]]': [[MSG]]

--- !dxcontainer
Header:
Hash: [ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
0x0, 0x0, 0x0, 0x0, 0x0, 0x0 ]
Version:
Major: 1
Minor: 0
FileSize: 108
PartCount: 3
PartOffsets: [ 60, 68, 76 ]
Parts:
- Name: FKE0
Size: 0
- Name: FKE1
Size: 0
- Name: FKE2
Size: 8
...
Loading