Page MenuHomePhabricator

[lldb] [Process/NetBSD] Fix wrongly mapping mm* registers
ClosedPublic

Authored by mgorny on Apr 5 2019, 9:58 AM.

Details

Summary

Fix mistake that mapped mm* registers into the space for xmm* registers,
rather than the one shared with st* registers. In other words,
'register read mmN' now correctly shows the mmN register rather than
part of xmmN.

Includes a minimal lit regression test.

Diff Detail

Event Timeline

mgorny created this revision.Apr 5 2019, 9:58 AM
krytarowski accepted this revision.Apr 5 2019, 1:00 PM
This revision is now accepted and ready to land.Apr 5 2019, 1:00 PM
labath added a comment.Apr 8 2019, 1:03 AM

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.)

lldb/lit/Register/x86-mm-xmm-read.test
1

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.

7–22

Maybe you could drop the -NEXTs here (or even replace them by -DAGs), because (I presume) you are not interested in the order the registers get printed, just their values.

mgorny marked 2 inline comments as done.Apr 8 2019, 10:31 AM

Thanks. I'll try converting it to C now.

lldb/lit/Register/x86-mm-xmm-read.test
1

Do you have any idea how to achieve the former? I can't find anything looking like it. Though I suppose it's entirely possible that there's no such thing atm.

mgorny updated this revision to Diff 194178.Apr 8 2019, 11:17 AM

Rewritten the test to C++ with inline assembly, and switched to CHECK-DAG. Still need to figure out how to filter it for correct platform.

labath added a subscriber: zturner.Apr 9 2019, 12:45 AM
labath added inline comments.
lldb/lit/Register/x86-mm-xmm-read.test
1

Yeah, it looks like there isn't anything like that at the moment. Maybe you could add something like system-x86 and use that in this test ?

+ @zturner, if he has any ideas here.

mgorny updated this revision to Diff 194363.Apr 9 2019, 10:46 AM

Updated 'REQUIRES'. I've looked through other LLVM tests, and noticed there's already 'native' concept (when host==target), and some 'target*' features for systems. I've chosen to combine the former with new 'target-x86'. I'll link the other patch in a minute.

mgorny updated this revision to Diff 194457.Apr 10 2019, 1:13 AM
labath accepted this revision.Apr 11 2019, 7:49 AM

The new version looks really great. Thank you.

As I mentioned previously, I think this will probably fail on windows, so I suggest you look out for the bot and be prepared to add an XFAIL here.

Will do, thanks.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2019, 7:58 AM