Page MenuHomePhabricator

Fix MIPS and MIPS64 ABI to use ConstString in thier register info arrays.
ClosedPublic

Authored by tatsuo on Sep 27 2020, 2:57 AM.

Details

Summary

RegInfoBasedABI::GetRegisterInfoByName was failing because mips/mips64 ABIs don't use ConstString in their register info array.

Diff Detail

Event Timeline

tatsuo created this revision.Sep 27 2020, 2:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2020, 2:57 AM
tatsuo requested review of this revision.Sep 27 2020, 2:57 AM
teemperor accepted this revision.Sep 27 2020, 3:14 AM
teemperor added a subscriber: teemperor.

To clarify the description a bit: RegInfoBasedABI::GetRegisterInfoByName is comparing the name/alt_name by doing a const char * pointer value comparison. That only works if both strings that are compared are coming from a ConstString. Since b0060c3a7868ef026d95d0cf8a076791ef74f474 GetRegisterInfoByName is checking with an assert that both C strings came from a ConstString (which is failing as these ABI implementation changed here are lacking the ConstString'ification code).

Obviously copying this code around isn't ideal, so this patch is just to get the backend at least running again until I get around to refactor this code.

Anyway, this LGTM.

This revision is now accepted and ready to land.Sep 27 2020, 3:14 AM

@teemperor Thanks for the quick review and context!
I don't have commit access, that'd be great if you could merge it in.

Done. Thanks for the patch!

This revision was landed with ongoing or failed builds.Sep 27 2020, 3:36 AM
This revision was automatically updated to reflect the committed changes.

Instead of copy-pasting this code can we extract a helper and call it with g_register_infos_mips64 and g_register_infos respectively?

Yeah, this is just to prevent the MIPS backend from hitting the assert (well, and get the related code to actually do what it's supposed to do). This whole code on my "to refactor" list.

labath added a subscriber: labath.Sep 29 2020, 5:08 AM

Not long ago. I added a MCBasedABI class which makes this goo mostly disappear as it gets its information from llvm MC layer. I haven't converted the mips abi due to a combination of being preempted by higher priority task, not having a mips machine to test on, and this code not being particularly egregious.

But if anyone wants to try this (and better yet, has ability to test it), I would strongly encourage converting these classes to an MCBasedABI. The only gotcha there is that the names of registers used by llvm mc and lldb/other tools don't always match, so it's sometimes necessary to write a small translation function. But that's still better than hundreds of lines of goo.