This is an archive of the discontinued LLVM Phabricator instance.

[Object][DX] Initial DXContainer parsing support
ClosedPublic

Authored by beanz on Apr 28 2022, 4:06 PM.

Details

Summary

This patch begins adding DXContainer parsing support to libObject.
Following the pattern used by ELFFile my goal here is to write a
standalone DXContainer parser and later write an adapter interface to
support a subset of the ObjectFile interfaces so that we can add
limited objdump support. I will also be adding ObjectYAML support to
help drive testing of the object tools and MC-level object writers as
those come together.

DXContainer is a slightly odd format. It is arranged in "parts" that
are semantically similar to sections, but it doesn't support symbol
listing.

Diff Detail

Unit TestsFailed

Event Timeline

beanz created this revision.Apr 28 2022, 4:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2022, 4:06 PM
beanz requested review of this revision.Apr 28 2022, 4:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2022, 4:06 PM
MaskRay added inline comments.Apr 28 2022, 6:12 PM
llvm/include/llvm/BinaryFormat/DXContainer.h
66

This comment just seems to duplicate what the code says.

If you just write uint8_t Magic[4]; // "DXBC", it seems to cover all the reader needs.

96

The comment doesn't convey more information than the code says.

llvm/lib/Object/DXContainer.cpp
38

Omit llvm::

beanz updated this revision to Diff 426082.Apr 29 2022, 9:06 AM

Updates based on PR feedback

beanz updated this revision to Diff 426083.Apr 29 2022, 9:07 AM

One more set of updates. Thanks @MaskRay!

MaskRay accepted this revision.Apr 30 2022, 12:49 PM
MaskRay added inline comments.
llvm/include/llvm/BinaryFormat/DXContainer.h
89

delete blank line

llvm/include/llvm/Object/DXContainer.h
22

Remove as the file doesn't use std::vector

llvm/lib/Object/DXContainer.cpp
17

Use static instead of anonymous namespace.

See https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

43

llvm::Error => Error everyewhere

llvm/unittests/Object/DXContainerTest.cpp
19

static

This revision is now accepted and ready to land.Apr 30 2022, 12:49 PM
This revision was landed with ongoing or failed builds.May 2 2022, 11:57 AM
This revision was automatically updated to reflect the committed changes.
beanz marked 2 inline comments as done.
hsmhsm added a subscriber: hsmhsm.May 2 2022, 12:36 PM
hsmhsm removed a subscriber: hsmhsm.
nikic added a subscriber: nikic.May 2 2022, 12:50 PM
nikic added inline comments.
llvm/include/llvm/BinaryFormat/DXContainer.h
68

This breaks the build with some versions of GCC:

In file included from /root/llvm-compile-time-tracker/llvm-project/llvm/include/llvm/Object/DXContainer.h:19,
                 from /root/llvm-compile-time-tracker/llvm-project/llvm/lib/Object/DXContainer.cpp:9:
/root/llvm-compile-time-tracker/llvm-project/llvm/include/llvm/BinaryFormat/DXContainer.h:67:8: error: declaration of ‘llvm::dxbc::Hash llvm::dxbc::Header::Hash’ changes meaning of ‘Hash’ [-fpermissive]
   67 |   Hash Hash;
      |        ^~~~
/root/llvm-compile-time-tracker/llvm-project/llvm/include/llvm/BinaryFormat/DXContainer.h:38:8: note: ‘Hash’ declared here as ‘struct llvm::dxbc::Hash’
   38 | struct Hash {
dyung added a subscriber: dyung.May 2 2022, 1:01 PM

The PS4 linux buildbot which runs gcc is not building any longer after this change, @beanz can you take a look?

https://lab.llvm.org/buildbot/#/builders/139/builds/21149

This isn't the only problem. There are also failures due to:

llvm-project/llvm/lib/Object/DXContainer.cpp:42:12: error: call to deleted constructor of 'llvm::Error'
    return Err;
           ^~~

See https://lab.llvm.org/buildbot/#/builders/21/builds/40034/steps/5/logs/stdio

This isn't the only problem. There are also failures due to:

llvm-project/llvm/lib/Object/DXContainer.cpp:42:12: error: call to deleted constructor of 'llvm::Error'
    return Err;
           ^~~

See https://lab.llvm.org/buildbot/#/builders/21/builds/40034/steps/5/logs/stdio

@beanz

I'm also seeing this build error when we try to merge this change into Apple's llvm-project fork.

/am_github_build_tester/llvm-project/llvm/lib/Object/DXContainer.cpp:42:12: error: call to deleted constructor of 'llvm::Error'
    return Err;
           ^~~
/am_github_build_tester/llvm-project/llvm/include/llvm/Support/Error.h:184:3: note: 'Error' has been explicitly marked deleted here
  Error(const Error &Other) = delete;
  ^
/am_github_build_tester/llvm-project/llvm/include/llvm/Support/Error.h:491:18: note: passing argument to parameter 'Err' here
  Expected(Error Err)
                 ^
1 error generated.

Could you take a look? It looks like the copy constructor and assignment operator are deliberately deleted. However the move constructor and move assignment operator are implemented so I'm guessing you might be able to do

if (Error Err = Container.parseHeader())
   return std::move(Err);

I'm a little surprised by this error though because the pattern you're using is what's in the llvm:Error documentation: https://llvm.org/doxygen/classllvm_1_1Error.html . It kind of suggests those comments are out-of-date.

The other issue should be fixed in rGb26e44e623c75c084e865084b18541c6a1736df2.

@beanz Thanks for fixing so quickly :)