Page MenuHomePhabricator

elf-core: Convert remaining register context to use register set maps
ClosedPublic

Authored by labath on Nov 16 2017, 8:15 AM.

Details

Summary

In D39681, we started using a map instead passing a long list of
register sets to the ppc64le register context. However, existing
register sets were still using the old method.

This converts remaining register contexts to use the register set map.
While doing this, I realized that using OS-specific keys for the map was
not a good idea, as sometimes we have a register context shared between
two OSs (e.g. linux and freebsd share the arm/arm64 register contexts).
Therefore, I create a new enum to represent register sets in an
OS-independent manner (the enum contains the generic GPR and FPR values,
but the rest of the values are likely to be architecture-specific).

This saves a bit of code now, but the real impact I see here is that it
can potentially pave the way for reduction of the number of register
context classes (for example RegisterContextPOSIXCore_arm and
RegisterContextPOSIXCore_arm64 are identical except that we initialize
them with a different RegisterInfoInterface object).

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Nov 16 2017, 8:15 AM
krytarowski edited edge metadata.Nov 16 2017, 10:18 AM

Looks reasonable!

I'm just thinking whether change code like CoreRegset::PPC_VMX to CoreRegset::PPC::VMX. I'm planning to add CoreRegset::X86_64::XSAVE in future.

alexandreyy edited edge metadata.Nov 16 2017, 10:29 AM

Looks good to me!

I'm just thinking whether change code like CoreRegset::PPC_VMX to CoreRegset::PPC::VMX. I'm planning to add CoreRegset::X86_64::XSAVE in future.

That's an interesting idea, but I don't see how would that be possible -- enum values can't have nested names. I suppose we could fake that by making CoreRegset and CoreRegset::X86_64 namespaces, but then the name of the enum type would have to be something different (CoreRegset::Type?). I am not sure if that's worth the trouble.

I don't think we will have that many register sets, so it's probably fine to have flat namespaces for arch/regset combinations.

krytarowski accepted this revision.Nov 17 2017, 3:02 AM
This revision is now accepted and ready to land.Nov 17 2017, 3:02 AM
clayborg added inline comments.Nov 17 2017, 9:54 AM
source/Plugins/Process/elf-core/elf-core-enums.h
58 ↗(On Diff #123189)

Seems weird to have PPC_VMX and PPC_VSX define in a CoreRegSet? Do these need to be specific for each arch? Why is everyone trying to use these?

labath added inline comments.Nov 17 2017, 1:39 PM
source/Plugins/Process/elf-core/elf-core-enums.h
58 ↗(On Diff #123189)

This enum represents all possible register sets that we can find in the elf core files. Not all register contexts are expected to use all of them -- each register context just plucks out those that he knows about (if they were present in the core file).

The enum (and the corresponding map) are intended as a replacement for the list of member variable register sets in the ThreadData struct. vregset (and the previous ppc64 patch was about to add one more) is already quite architecture-specific.

While I do think that this is an improvement over the list-of-members solution, I am also not entirely happy with having these arch-specific sets in the enum.

The alternative I thought of just now is to have a a plain vector instead of a map for register set data. Then, at the generic level, we will just be dealing with "register set 0", "register set 1", etc.. and when we consult the RegisterInfoInterface class, it will tell us that "register set 0" is in fact GPR, register set 2 is VMX, etc.

What do you think?

clayborg added inline comments.Nov 17 2017, 2:09 PM
source/Plugins/Process/elf-core/elf-core-enums.h
58 ↗(On Diff #123189)

I like the vector idea a bit better and I like the idea of the RegisterInfoInterface telling us that "register set at index 0" is GPR, etc.

labath planned changes to this revision.Nov 21 2017, 7:53 AM

I am going to come back to this later, with a slightly different approach, but first I need to do another cleanup elsewhere.

labath updated this revision to Diff 124064.Nov 23 2017, 4:07 AM

This is a slight deviation from the initial approach. What I've done here is
that I've kept the register set indices in an os-specific form. I've done this
because it enables the parsing code in ProcessElfCore to be
architecture-agnostic -- it can just deal with OS specifics, and can just shove
any note it does not recognise to the side. These will be then parsed in the
corresponding register contexts, which can only deal with architecture
specifics, and ignore OS details, as the translation of os-specific note types
is provided by a separate lookup table.

This way, adding an register set, which is already supported on other OSes, to a
new OS, should in most cases be as simple as adding a new entry into the
register set description map.

This revision is now accepted and ready to land.Nov 23 2017, 4:07 AM
clayborg accepted this revision.Nov 27 2017, 9:47 AM
This revision was automatically updated to reflect the committed changes.