[PATCH] D85334: [llvm-libtool-darwin] Support universal outputs

Wed Aug 12 09:50:36 PDT 2020

sameerarora101 marked 3 inline comments as done.
sameerarora101 added inline comments.

================
Comment at: llvm/test/tools/llvm-libtool-darwin/universal-output.test:4-5
+
+# RUN: yaml2obj %s --docnum=1 -o %t.armv6
+# RUN: yaml2obj %s --docnum=2 -o %t.armv7
+
----------------
jhenderson wrote:
> Rather than having two near-identical YAML blobs, use the `-D` option to pull out the different bits:
> 
> ```
> # RUN: yaml2obj %s --docnum=1 -o %t.armv6 -DSUBTYPE=0x6 ...
> # RUN: yaml2obj %s --docnum=2 -o %t.armv7 -DSUBTYPE=0x9 ...
> ```
> 
> There's a lot of extra noise in the two, which I suspect you can get rid of with a bit of effort. You don't need to have the YAML directly represent a real object you've made, as long as it's sufficient to trigger the code paths you need. For example, do you actually need the segment and section contents?
Thanks, I was able to get rid of section contents as well. However, I still need the segment command or it throws an "invalid object file" error (perhaps because of  the string table below?). Updated it now.

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:123

+static uint64_t getCPUID(uint32_t CPUType, uint32_t CPUSubtype) {
+  switch (CPUType) {
----------------
jhenderson wrote:
> Looks to me like this function is only partially tested? What about the other three CPUs and the default case?
Ok also added test for `CPU_TYPE_ARM64`,  `CPU_TYPE_X86_64` and the default case. `CPU_TYPE_ARM64_32` does not have a subtype so I didn't add a test for it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85334