[Lldb-commits] [PATCH] D60325: [lldb] [Process/NetBSD] Fix wrongly mapping mm* registers

Mon Apr 8 01:05:44 PDT 2019

labath added a comment.

I really like how you've approached testing this. There's just one thing that bothers me. Using a .s file means it's going to be hard to make this test work on other OSs (and we should be able to test something like this in a cross-OS manner). All elf targets should probably be fine, but I am pretty sure darwin and windows will choke on this.

I think it would be better to write this as a C(++) file and move the register setting code to inline asm. That should be more-or-less supported on all platforms (when compiling with clang). You should be able to test that by trying a cross-compilation with clang (`clang -target x86_64-pc-windows -c foo.c`). I've tried it with a simplified snipped from your test and it seemed to work fine.

(Note that I still expect this test to fail on windows, because AFAIK we don't support reading these registers on windows yet, but it would be good if at least the source code compiled there.)

================
Comment at: lldb/lit/Register/x86-mm-xmm-read.test:1
+# REQUIRES: x86
+# RUN: %clang %p/Inputs/x86-mm-xmm-read.s -o %t
----------------
Does this mean "the host is an x86 system", or "x86 is a configured llvm target"? My impression is that in means the latter <https://github.com/llvm-mirror/lldb/blob/master/lit/lit.cfg.py#L49>, but we obviously want the former here.

================
Comment at: lldb/lit/Register/x86-mm-xmm-read.test:7-22
+# CHECK: mm0 = 0x0102030405060708
+# CHECK-NEXT: mm1 = 0x1112131415161718
+# CHECK-NEXT: mm2 = 0x2122232425262728
+# CHECK-NEXT: mm3 = 0x3132333435363738
+# CHECK-NEXT: mm4 = 0x4142434445464748
+# CHECK-NEXT: mm5 = 0x5152535455565758
+# CHECK-NEXT: mm6 = 0x6162636465666768
----------------
Maybe you could drop the `-NEXT`s here (or even replace them by `-DAG`s), because (I presume) you are not interested in the order the registers get printed, just their values.

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

https://reviews.llvm.org/D60325