[PATCH] D126795: [DX][ObjYAML] Support for parsing DXIL part

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/DXContainer.h:125
+
+static_assert(sizeof(ProgramHeader) == 24, "ProgramHeader Size incorrect!");
+
----------------
beanz wrote:
> kuhar wrote:
> > It seems to me that this relies on the structs being tightly packed. But does it always have to be the case? I thought that the compiler is allowed to insert padding arbitrarily as long as all fields remain aligned.
> > 
> > In the serialization/deserialization code I worked on in the past, we always relied on the size of individual fields for this reason, and always read/wrote all structs field wise.
> In C struct members are "aligned in an implementation-defined manner appropriate to its type". The compiler can't arbitrarily add padding, but it can to ensure appropriate alignment. I explicitly added an `Unused` member to represent the padding that is expected, and that should make the remaining structure layout consistent with every architecture I'm aware of.
> 
> The assert is just intended to verify my bit math (and that none of our other architectures do anything weird) because code depends on it.
> 
> For binary file formats like object files we pretty regularly rely on structure definitions being C-standard layout. This is consistent throughout the BinaryFormat library.
I think that's fair. It's very unlikely for an implementation to over-align fields and a lot of other code would break too.


================
Comment at: llvm/lib/Object/DXContainer.cpp:13
 
+#include <stddef.h>
+
----------------
nit: `cstddef`? I think  we generally prefer C++ versions of C headers


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126795/new/

https://reviews.llvm.org/D126795