Skip to content

[DirectX] Add support for remove-section of DXContainer for llvm-objcopy #153246

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

Open
wants to merge 8 commits into
base: users/inbelic/pr-153079
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions llvm/lib/ObjCopy/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ source_group("Source Files\\DXContainer" REGULAR_EXPRESSION
add_llvm_component_library(LLVMObjCopy
Archive.cpp
DXContainer/DXContainerObjcopy.cpp
DXContainer/DXContainerObject.cpp
DXContainer/DXContainerReader.cpp
DXContainer/DXContainerWriter.cpp
CommonConfig.cpp
Expand Down
16 changes: 8 additions & 8 deletions llvm/lib/ObjCopy/ConfigManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,14 @@ ConfigManager::getDXContainerConfig() const {
!Common.AllocSectionsPrefix.empty() ||
Common.DiscardMode != DiscardType::None || !Common.AddSection.empty() ||
!Common.DumpSection.empty() || !Common.KeepSection.empty() ||
!Common.OnlySection.empty() || !Common.ToRemove.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.OnlySection.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,
Expand Down
13 changes: 13 additions & 0 deletions llvm/lib/ObjCopy/DXContainer/DXContainerObjcopy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,19 @@ namespace dxbc {
using namespace object;

static Error handleArgs(const CommonConfig &Config, Object &Obj) {
std::function<bool(const Part &)> RemovePred = [](const Part &) {
return false;
};

if (!Config.ToRemove.empty())
RemovePred = [&Config](const Part &P) {
return Config.ToRemove.matches(P.Name);
};

if (auto E = Obj.removeParts(RemovePred))
return E;

Obj.recomputeHeader();
return Error::success();
}

Expand Down
29 changes: 29 additions & 0 deletions llvm/lib/ObjCopy/DXContainer/DXContainerObject.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
//===- DXContainerObject.cpp ----------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#include "DXContainerObject.h"

namespace llvm {
namespace objcopy {
namespace dxbc {

Error Object::removeParts(PartPred ToRemove) {
erase_if(Parts, ToRemove);
return Error::success();
}

void Object::recomputeHeader() {
Header.FileSize = headerSize();
Header.PartCount = Parts.size();
for (const Part &P : Parts)
Header.FileSize += P.size();
}

} // end namespace dxbc
} // end namespace objcopy
} // end namespace llvm
5 changes: 5 additions & 0 deletions llvm/lib/ObjCopy/DXContainer/DXContainerObject.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ struct Part {
}
};

using PartPred = llvm::function_ref<bool(const Part &)>;

struct Object {
::llvm::dxbc::Header Header;
SmallVector<Part> Parts;
Expand All @@ -38,6 +40,9 @@ struct Object {
return sizeof(::llvm::dxbc::Header) // base header
+ sizeof(uint32_t) * Parts.size(); // part offset values
}

Error removeParts(PartPred ToRemove);
void recomputeHeader();
};

} // end namespace dxbc
Expand Down
2 changes: 1 addition & 1 deletion llvm/test/tools/llvm-objcopy/DXContainer/headers-copy.test
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ Header:
Version:
Major: 1
Minor: 0
FileSize: 3548
FileSize: 1996
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this different if the remove section arg wasn't passed to llvm-objcopy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, this commit was meant to be pushed to #153079.

In any case, the yaml here incorrectly specified the FileSize as 3548 instead of 1996. Since we now invoke recomputeHeader as part of removeParts it was failing this test case, because the (correctly) computed size was different than the original specified.

PartCount: 7
PartOffsets: [ 60, 76, 92, 108, 236, 1932, 1960 ]
Parts:
Expand Down
50 changes: 50 additions & 0 deletions llvm/test/tools/llvm-objcopy/DXContainer/remove-headers.test
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
## Tests that the copied DXContainer correctly has the specified headers
## removed.

# RUN: yaml2obj %s -o %t
# RUN: llvm-objcopy --remove-section=FKE1 --remove-section=FKE4 %t %t.out
# RUN: obj2yaml %t.out | FileCheck %s

--- !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 = 1996 - 8 (FKE1 content) - 1688 (FKE4 content)
## - 8 (2 part offsets) - 16 (2 part headers)
## = 276
# CHECK: FileSize: 276
FileSize: 1996
# CHECK: PartCount: 5
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can any of these be CHECK-NEXT:?

PartCount: 7
# CHECK: PartOffsets: [ 52, 68, 84, 212, 240 ]
PartOffsets: [ 60, 76, 92, 108, 236, 1932, 1960 ]
Parts:
# CHECK-NOT: FKE1
# CHECK-NOT: FKE4
Comment on lines +25 to +26
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you aware that this just means FKE1 and FKE4 can't appear from this point onwards? I'd be tempted to put a # CHECK: Parts: line before them, since technically the obj2yaml output order doesn't have to be fixed and this will help remove the risk of things rotting.

- Name: FKE0
Size: 8
- Name: FKE1
Size: 8
- Name: FKE2
Size: 8
- Name: FKE3
Size: 120
- Name: FKE4
Size: 1688
- Name: FKE5
Size: 20
- Name: DXIL
Size: 28
Program:
MajorVersion: 6
MinorVersion: 5
ShaderKind: 5
Size: 8
DXILMajorVersion: 1
DXILMinorVersion: 5
DXILSize: 4
DXIL: [ 0x42, 0x43, 0xC0, 0xDE, ]
...
Loading