[PATCH] D65799: [yaml2obj/obj2yaml][MachO] Allow setting custom section data

Wed Aug 7 22:32:44 PDT 2019

seiya marked an inline comment as done.
seiya added inline comments.

================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:1948

+ArrayRef<uint8_t> MachOObjectFile::getSectionContents(uint32_t Offset,
+                                                      uint64_t Size) const {
----------------
jhenderson wrote:
> A 32-bit Offset but 64-bit Size seems off to me. What are the limitations on the two fields in Mach-O's file format?
This comes from the definition of `struct section_64` [1].

[1]: https://github.com/opensource-apple/cctools/blob/master/include/mach-o/loader.h#L427-L428

================
Comment at: llvm/test/ObjectYAML/MachO/virtual_section.yaml:178
 # CHECK-NEXT:        reserved3:       0x00000000
-# CHECK-NEXT:      - sectname:        __data
+# CHECK:           - sectname:        __data
 # CHECK-NEXT:        segname:         __DATA
----------------
jhenderson wrote:
> Why are these no longer CHECK-NEXT? Should you be testing the new stuff before it?
I removed the `-NEXT` because the newly added `content` fields were lengthy but it was  bad idea. I've replaced them with regular expressions instead. 

================
Comment at: llvm/tools/obj2yaml/macho2yaml.cpp:21

+static bool isVirtualSection(uint8_t type) {
+  return (type == MachO::S_ZEROFILL || type == MachO::S_GB_ZEROFILL ||
----------------
I'd like to move this function into somewhere else instead of copying from MachOEmitter.cpp but I couldn't locate the appropriate place. Any suggestions?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65799