[PATCH] D141848: [Test] Fix YAML mapping keys duplication. NFC.

Mon Feb 6 02:05:05 PST 2023

asi-sc added a subscriber: steven_wu.
asi-sc added a comment.

@yota9 , @cishida , @spowell  ,  @steven_wu , sorry for mentioning you directly in this review, however I believe you are the right experts to consult about the content of some tests. We need to get rid of keys duplication in YAML inputs as keys duplication is not allowed by the specification. These fixes in tests blocks improvement of LLVM's YAML parser. Could you please check the comments below where I've mentioned you?

================
Comment at: bolt/test/AArch64/Inputs/plt-gnu-ld.yaml:178
         Addend:          56
+      - Offset:          0x400540
         Symbol:          '__libc_start_main@@GLIBC_2.17'
----------------
Currently, YAML parser seems to overwrite Symbol (`.text` -> `'__libc_start_main@@GLIBC_2.17'`)  and Type (`R_AARCH64_ADD_ABS_LO12_NC` -> `R_AARCH64_CALL26`) values at Offset `0x40052C`. And the test works just because we are lucky. I think the right fix is to introduce an offset for `__libc_start_main@@GLIBC_2.17` symbol, but I'm not sure in the offset's value I've chosen. @yota9 , may I ask you to take a quick look at this fix?

================
Comment at: llvm/unittests/TextAPI/TextStubV4Tests.cpp:984
+      "  - targets: [ x86_64-maccatalyst ]\n"
+      "    symbols: [ _symAB ]\n"
       "reexports:\n"
----------------
@spowell , @cishida , @steven_wu , `TBDv4File` file seems to contain two files incorrectly merged into one. I've merged some fields so that it doesn't contradict YAML spec, but I'm not sure in the correctness of this change. If such a merging is not possible, when we must split one file into two. However, looking at the checks, it doesn't sound as a required thing for this test. Could you please have a look? 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141848