This is an archive of the discontinued LLVM Phabricator instance.

Move architecture-specific address adjustment to architecture plugins.
ClosedPublic

Authored by tatyana-krasnukha on Jun 27 2018, 12:29 AM.

Event Timeline

Removed unrelated changes from the patch.

I doubt that I should create plugin for thumb also. Cannot ArchitectureArm handle it?
And what about big-endian versions?

source/Plugins/Architecture/Mips/ArchitectureMips.cpp
39

Is it ok to have single architecture plugin for all these machines?

source/Target/Target.cpp
2359

Supposed this is fair regardless of architecture.

labath added inline comments.Jun 27 2018, 1:14 AM
include/lldb/Core/Architecture.h
77–79

The rest of the codebase doesn't use the /*commented_out_argument*/ style.

source/Plugins/Architecture/Mips/ArchitectureMips.cpp
38–42

Use ArchSpec::IsMIPS() ?

source/Target/Target.cpp
2369

Why do we need the DoesSupport call? Could we just have the default implementation of GetAddressFlagMask return 0?

I doubt that I should create plugin for thumb also. Cannot ArchitectureArm handle it?

I don't think a special thumb plugin would be good, as you can have mixed arm&thumb code within a single process.

And what about big-endian versions?

Let's try to avoid them for now. Hopefully the differences (if any) can be handled by simple helper functions.

source/Plugins/Architecture/Mips/ArchitectureMips.cpp
39

I think that is fine for the time being. We can always revisit that if the implementations start to diverge.

source/Target/Target.cpp
2369

If target doesn't support this trick with special bits in address, it may need aligned callable addresses as well. In this case we fall back to GetOpcodeLoadAddress.

labath added inline comments.Jun 27 2018, 1:44 AM
source/Target/Target.cpp
2369

Hmm... why is that fallback there in the first place? I don't see anything like that in the original code. I thought this was meant to be NFC.

source/Target/Target.cpp
2369

If user executes 'thread jump' command with unaligned address, last will be written to PC register as is and I'm not sure that things will be good after that. May be over-caution.

Uncommented arguments and used ArchSpec::IsMIPS()

clayborg requested changes to this revision.Jun 27 2018, 7:18 AM

Instead of adding a bunch of calls to the Architecture for feature flags and masks and instructions, just ask the architecture plug-in to do the work you need them to do. See inlined comments,

include/lldb/Core/Architecture.h
17–18

#include lldb-forward.h for this kind of stuff.

73–74

Remove Feature and DoesSupport(). Just add GetCallableLoadAddress(...) and GetBreakableLoadAddress(...) to the architecture plug-in.

76–81

Remove. See above comment.

83–86

Remove, see above comments

source/Target/Target.cpp
383

revert

389

revert

2369

Add a "GetCallableLoadAddress(...)" function to the Architecture plug-in. Remove the Architecture::GetAddressFlagMask().

2384–2385

Just let architecture plugin do this if needed inside Architecture::GetCallableLoadAddress(...)

2391–2393

Just add a Architecture::GetBreakableLoadAddress() and remove all arch specific functionality in this fucntion.

This revision now requires changes to proceed.Jun 27 2018, 7:18 AM
tatyana-krasnukha marked an inline comment as done.

Upd: Making functions static with Target* parameter allowed to rid out of checking target and architecture plugin by caller code.

clayborg requested changes to this revision.Jul 18 2018, 9:47 AM

Great patch, but I would still rather ask the target to do this for us rather than having a static function on a different class that must take a Target as the first argument.

include/lldb/Core/Architecture.h
85

remove static from all these and make virtual

89–101

Remove all these Do calls

This revision now requires changes to proceed.Jul 18 2018, 9:47 AM

@clayborg , thank you for the feedback, but why don't you like the approach with static functions?

I see a few advantages in it:

  • no need to duplicate code that is common for all architectures; from the other hand, no need to call base class's function from overridden one (that is easy to forget)
  • function call
auto code_addr = Architecture::GetCallableLoadAddress(target_ptr, addr, AddressClass::eCode);

looks more readable against

 addr_t code_addr = addr;
 if (target_ptr) {  
    auto arch = target->GetArchitecturePlugin();
    if (arch)
        code_addr = arch->GetCallableLoadAddress(addr, AddressClass::eCode);
}

and is safer, because there are places where caller doesn't check pointers.

  • and finally, it is consistent with existing interface of Address class, whose GetCallableLoadAddress and GetOpcodeLoadAddress take a Target as the first argument as well.

Also, GetBreakableLoadAddress needs a Target anyway.

I would be grateful if you could point where my thoughts are wrong.

source/Core/Address.cpp
356 ↗(On Diff #154720)

unsafe

source/Target/RegisterContext.cpp
137 ↗(On Diff #154720)

too safe;)

@clayborg , thank you for the feedback, but why don't you like the approach with static functions?

I see a few advantages in it:

  • no need to duplicate code that is common for all architectures; from the other hand, no need to call base class's function from overridden one (that is easy to forget)
  • function call
auto code_addr = Architecture::GetCallableLoadAddress(target_ptr, addr, AddressClass::eCode);

looks more readable against

 addr_t code_addr = addr;
 if (target_ptr) {  
    auto arch = target->GetArchitecturePlugin();
    if (arch)
        code_addr = arch->GetCallableLoadAddress(addr, AddressClass::eCode);
}

This code would actually be:

addr_t code_addr = addr;
if (target_ptr) 
  code_addr = target->GetCallableLoadAddress(addr, AddressClass::eCode);

Any code that is still checking both the target and the arch as you above example was doing should just be asking the target to do this, we can add functions to lldb_private::Target if needed.

and is safer, because there are places where caller doesn't check pointers.

Callers should always be checking pointers.

  • and finally, it is consistent with existing interface of Address class, whose GetCallableLoadAddress and GetOpcodeLoadAddress take a Target as the first argument as well.

The address object is still being asked do something:

addr.GetCallableLoadAddress(target, ...)

Not:

Address::GetCallableLoadAddress(addr, target, ...);

Also, GetBreakableLoadAddress needs a Target anyway.

I would be grateful if you could point where my thoughts are wrong.

I just like to keep this object oriented by asking the object we want to do something for us. Doing the static function solution seems like a C kind of thing that is being done in C++

tatyana-krasnukha retitled this revision from WIP: Move architecture-specific address adjustment to architecture plugins. to Move architecture-specific address adjustment to architecture plugins..

Updated according to comments, sorry for long delay.

Make overriding functions public.

So I would suggest not removing any functions from lldb_private::Target, just change the old ones to call through the arch plug-ins if there is one. Then many changes in this diff just go away and we still get the clean implementation where things are delegated to the Architecture plug-ins

include/lldb/Target/Target.h
750 ↗(On Diff #166475)

Why did we remove these calls from target? We should have default implementations that do nothing if there is no arch plug-in, and call through the arch plug-in if there is one. As I mentioned we don't want people to have to check if the arch plug-in is valid at each call site.

source/Core/Address.cpp
333 ↗(On Diff #166475)

None of the changes in Address.cpp are needed if we leave the functions in Target.

source/Target/RegisterContext.cpp
132–140 ↗(On Diff #166475)

None of these changes are needed if we leave the function in Target.h/.cpp

source/Target/Target.cpp
-148–1

None of these changes are needed if we leave the function in Target.h/.cpp

0–1

Leave this function in and call through to arch plug-in if we have one.

0–1

Leave this function in and call through to arch plug-in if we have one.

0–1

Leave this function in and call through to arch plug-in if we have one.

source/Target/ThreadPlanRunToAddress.cpp
45–47 ↗(On Diff #166475)

None of these changes are needed if we leave the function in Target.h/.cpp

59–63 ↗(On Diff #166475)

None of these changes are needed if we leave the function in Target.h/.cpp

clayborg accepted this revision.Sep 21 2018, 11:40 AM

much better! Thanks for making the changes

This revision is now accepted and ready to land.Sep 21 2018, 11:40 AM

Thanks for your patience;)

This revision was automatically updated to reflect the committed changes.