Page MenuHomePhabricator

[lldb] Remove all the RegisterInfo name constification code
ClosedPublic

Authored by teemperor on Sep 29 2020, 7:46 AM.

Details

Summary

RegisterInfo's reg_name/reg_alt_name fields are C-Strings and are supposed to only
be generated from a ConstString. The reason for that is that DynamicRegisterInfo::GetRegisterInfo
and RegInfoBasedABI::GetRegisterInfoByName try to optimise finding registers by name
by only comparing the C string pointer values instead of the underlying strings. This only
works if both C strings involved in the comparison come from a ConstString. If one of the
two C strings doesn't come from a ConstString the comparison won't work (and most likely
will silently fail).

I added an assert in b0060c3a7868ef026d95d0cf8a076791ef74f474 which checks that
both strings come from a ConstString. Apparently not all ABI plugins are generating their
register names via ConstString, so this code is now not just silently failing but also asserting.

In D88375 we did a shady fix for the MIPS plugins by just copying the ConstString setup code to that
plugin, but we still need to fix ABISysV_arc, ABISysV_ppc and ABISysV_ppc64 plugins.

I would say we just fix the remaining plugins by removing the whole requirement to have the
register names coming from ConstStrings. I really doubt that we actually save any time with
the whole ConstString search trick (searching ~50 strings that have <4 characters doesn't sound more
expensive than calling the really expensive ConstString constructor + comparing the same amount
of pointer values). Also whatever small percentage of LLDB's runtime is actually spend in this
function is anyway not worth the complexity of this approach.

This patch just removes all this and just does a normal string comparison.

Diff Detail

Event Timeline

teemperor created this revision.Sep 29 2020, 7:46 AM
teemperor requested review of this revision.Sep 29 2020, 7:46 AM

I saw Pavel's comment about MCBasedABI in D88375 when this was already done, so I'll just put this up here. Not sure if I'll get around (or even be the right person) for doing that conversion, but I guess this patch is straightforward code removal that gets rid of some technical debt, so it's at least a step in the right direction.

labath accepted this revision.Sep 29 2020, 7:58 AM

Yeah, this is good stuff independently of the MCBasedABI business.

This revision is now accepted and ready to land.Sep 29 2020, 7:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 13 2020, 8:10 AM