Differential D44429
[BinaryFormat] MessagePack reader/writer Authored by scott.linder on Mar 13 2018, 8:00 AM.
Details Add support for reading and writing MessagePack, a binary object serialization format which aims to be more compact than text formats like JSON or YAML. The specification can be found at https://github.com/msgpack/msgpack/blob/master/spec.md AMD is defining our next code object metadata format in terms of MessagePack, and the linked patches reflect that use.
Diff Detail Event TimelineComment Actions @chandlerc Have you had a chance to look at the patch? Do you know anyone else who would be interested? Comment Actions I haven't really had time to look at this, but it seems somewhat unfortunate to add yet another format like this. =[ I guess I'm surprised that AMD is looking at baking this into their object file format. I'm not sure it makes sense to really consider landing this until the usage of it is at least available for review as well. While having the patches separate is good, I think it would be important to see the actual planned usage before adding all this code. @dblaikie may have comments about the layering and the unittesting.
Comment Actions Thank you for taking a look and adding subscribers; I didn't know who might have interest. I understand the aversion to merging unused code, I just wanted to get feedback on the possibility as soon as possible. As for supporting another format I also understand the reluctance, but the MessagePack specification seems to be fairly static, and the code implementing it is relatively concise and straightforward. I do not think this code will have a significant impact on maintenance. I am beginning work now on code for the AMD object file which will use MessagePack, so I should have more context available for this patch soon.
Comment Actions Addressed feedback wrt. layering and testing. Moved library code from Support to BinaryFormat. Comment Actions Testing is verbose, but probably suitable given the API involved. Layering - looks about right, though if this is only used by one place, it should just go there rather than in a more common library like this.
Comment Actions I have the library in BinaryFormat because it is used outside of the AMDGPU backend in tools (currently just llvm-readobj) in https://reviews.llvm.org/D48179
Comment Actions Causes a regression with modular builds here (All other parallel build tasks failed on different parts of building the BinaryFormat module, this is just the first one): [168/3291] Building CXX object lib/BinaryFormat/CMakeFiles/LLVMBinaryFormat.dir/Dwarf.cpp.o
FAILED: lib/BinaryFormat/CMakeFiles/LLVMBinaryFormat.dir/Dwarf.cpp.o
/usr/local/sdk/llvm-8.0.4078/bin/clang++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/BinaryFormat -I/SourceCache/llvm-trunk-8.0.4079/lib/BinaryFormat -Iinclude -I/SourceCache/llvm-trunk-8.0.4079/include -O3 -march=skylake -dwarf-4 -rtlib=compiler-rt -Wno-unused-command-line-argument -stdlib=libc++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -fmodules -fmodules-cache-path=/o/org.llvm.caches/llvm-8.0/4079/module.cache -Xclang -fmodules-local-submodule-visibility -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -flto=thin -O3 -UNDEBUG -fno-exceptions -fno-rtti -MD -MT lib/BinaryFormat/CMakeFiles/LLVMBinaryFormat.dir/Dwarf.cpp.o -MF lib/BinaryFormat/CMakeFiles/LLVMBinaryFormat.dir/Dwarf.cpp.o.d -o lib/BinaryFormat/CMakeFiles/LLVMBinaryFormat.dir/Dwarf.cpp.o -c /SourceCache/llvm-trunk-8.0.4079/lib/BinaryFormat/Dwarf.cpp
While building module 'LLVM_BinaryFormat' imported from /SourceCache/llvm-trunk-8.0.4079/lib/BinaryFormat/Dwarf.cpp:14:
In file included from <module-includes>:6:
/SourceCache/llvm-trunk-8.0/4079/include/llvm/BinaryFormat/MsgPack.h:30:1: error: redundant #include of module 'LLVM_BinaryFormat.MsgPack' appears within namespace 'llvm::msgpack::FirstByte' [-Wmodules-import-nested-redundant]
#include "llvm/BinaryFormat/MsgPack.def"
^
/SourceCache/llvm-trunk-8.0/4079/include/llvm/BinaryFormat/MsgPack.h:28:1: note: namespace 'llvm::msgpack::FirstByte' begins here
namespace FirstByte {
^
/SourceCache/llvm-trunk-8.0/4079/include/llvm/BinaryFormat/MsgPack.h:42:1: error: redundant #include of module 'LLVM_BinaryFormat.MsgPack' appears within namespace 'llvm::msgpack::FixBits' [-Wmodules-import-nested-redundant]
#include "llvm/BinaryFormat/MsgPack.def"
^
/SourceCache/llvm-trunk-8.0/4079/include/llvm/BinaryFormat/MsgPack.h:40:1: note: namespace 'llvm::msgpack::FixBits' begins here
namespace FixBits {
^
/SourceCache/llvm-trunk-8.0/4079/include/llvm/BinaryFormat/MsgPack.h:55:1: error: redundant #include of module 'LLVM_BinaryFormat.MsgPack' appears within namespace 'llvm::msgpack::FixBitsMask' [-Wmodules-import-nested-redundant]
#include "llvm/BinaryFormat/MsgPack.def"
^
/SourceCache/llvm-trunk-8.0/4079/include/llvm/BinaryFormat/MsgPack.h:53:1: note: namespace 'llvm::msgpack::FixBitsMask' begins here
namespace FixBitsMask {
^
/SourceCache/llvm-trunk-8.0/4079/include/llvm/BinaryFormat/MsgPack.h:64:1: error: redundant #include of module 'LLVM_BinaryFormat.MsgPack' appears within namespace 'llvm::msgpack::FixMax' [-Wmodules-import-nested-redundant]
#include "llvm/BinaryFormat/MsgPack.def"
^
/SourceCache/llvm-trunk-8.0/4079/include/llvm/BinaryFormat/MsgPack.h:62:1: note: namespace 'llvm::msgpack::FixMax' begins here
namespace FixMax {
^
/SourceCache/llvm-trunk-8.0/4079/include/llvm/BinaryFormat/MsgPack.h:74:1: error: redundant #include of module 'LLVM_BinaryFormat.MsgPack' appears within namespace 'llvm::msgpack::FixLen' [-Wmodules-import-nested-redundant]
#include "llvm/BinaryFormat/MsgPack.def"
^
/SourceCache/llvm-trunk-8.0/4079/include/llvm/BinaryFormat/MsgPack.h:72:1: note: namespace 'llvm::msgpack::FixLen' begins here
namespace FixLen {
^
/SourceCache/llvm-trunk-8.0/4079/include/llvm/BinaryFormat/MsgPack.h:87:1: error: redundant #include of module 'LLVM_BinaryFormat.MsgPack' appears within namespace 'llvm::msgpack::FixMin' [-Wmodules-import-nested-redundant]
#include "llvm/BinaryFormat/MsgPack.def"
^
/SourceCache/llvm-trunk-8.0/4079/include/llvm/BinaryFormat/MsgPack.h:85:1: note: namespace 'llvm::msgpack::FixMin' begins here
namespace FixMin {
^
/SourceCache/llvm-trunk-8.0/4079/lib/BinaryFormat/Dwarf.cpp:14:10: fatal error: could not build module 'LLVM_BinaryFormat'
#include "llvm/BinaryFormat/Dwarf.h"
~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
7 errors generated.Comment Actions If you're maintaining/watching the modules buildbot, this is probably the sort of thing worth fixing forward yourself - I don't think everyone's aware of/maintaining the modules build but those of us who do can fix things up easily enough - adding the .def file to the include/llvm/module.modulemap file along with all the other .def files that are already there. Comment Actions Don't have commit access but this should be revised to update the modulemap. module LLVM_BinaryFormat {
requires cplusplus
umbrella "BinaryFormat" module * { export * }
textual header "BinaryFormat/Dwarf.def"
// -snip-
textual header "BinaryFormat/WasmRelocs.def"
textual header "BinaryFormat/MsgPack.def"
}Modular build passes after that. Comment Actions Yes though unfortunately, since I lack commit access I can't commit minor changes like that, hence my request for revision. Comment Actions Thank you! Buildbot for next one (numbering is odd, I know, /o/org.llvm.caches/llvm-8.0/4080) passed. Comment Actions Hi, It looks like there's some UB in this code. Pls see this failure: https://reviews.llvm.org/D76140#1953201 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This looks to be a layering violation - BinaryFormat depends on Support, not the other way around. So Support can't/shouldn't include BinaryFormat headers. (similar/same issue with the implementations in lib/Support)