Page MenuHomePhabricator

Add specific ppc64le hardware watchpoint handler
ClosedPublic

Authored by anajuliapc on Oct 13 2017, 10:53 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

anajuliapc created this revision.Oct 13 2017, 10:53 AM
zturner added inline comments.
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp
90–93 ↗(On Diff #118942)

Can you set these inline in the class body? e.g. when you declare them in the header file:

uint32_t m_max_hwp_supported = 16;
uint32_t m_max_hbp_supported = 16;
bool m_refresh_hwdebug_info = true;
283–295 ↗(On Diff #118942)

Can you make an enum for these magic values 1, 2, and 3?

302–318 ↗(On Diff #118942)

How about:

addr_t begin = llvm::alignDown(addr, 8);
addr_t end = llvm::alignUp(addr+size, 8);
size = llvm::NextPowerOf2(end-begin);
327–333 ↗(On Diff #118942)

Can we use a range-based for here?

DREG *reg = nullptr;
for (auto &dr : m_hwp_regs) {
  if (dr.control & 1) == 0)
    reg = &dr;
  else if (dr.address == addr)
    return LLDB_INVALID_INDEX32;
}

if (!reg)
  return LLDB_INVALID_INDEX32;

reg->real_addr = real_addr;
reg->address = addr;
reg->control = control_value;
reg->mode = rw_mode;

etc

349 ↗(On Diff #118942)

How about:

m_hwp_regs[wp_index].control &= llvm::maskTrailingZeros<uint32_t>(1);
374 ↗(On Diff #118942)

reinterpret_cast from long* to long? Is this correct?

389 ↗(On Diff #118942)

Same as before. Is this correct?

402–413 ↗(On Diff #118942)
unsigned control = (m_hwp_regs[wp_index].control >> 5) & 0xFF;
assert(llvm::isPowerOf2_32(control + 1));
return llvm::countPopulation(control);
416–417 ↗(On Diff #118942)

Can this function accept a const DREG& instead of an index?

421–424 ↗(On Diff #118942)

return !!(m_hwp_regs[wp_index].control & 0x1);

474 ↗(On Diff #118942)

No need for else here.

source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h
70–72 ↗(On Diff #118942)

Can these two functions take const DREG& instead of indices? Also, it seems like they could be raised out of the class and into global static functions in the cpp file.

102 ↗(On Diff #118942)

How about std::array<DREG, 4> m_hwp_regs;

I agree with all of Zach's suggestions.

source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h
98–99 ↗(On Diff #118942)

As long as this code is only ever compiled native, using "long" and "int" are ok. Just beware of code that isn't always compiled native.

Feel free to add me as a reviewer. Zach as well.

gut added a comment.Oct 13 2017, 1:35 PM

Thanks for supporting this change. I guess @anajuliapc will add you both as reviewer as soon as she updates this patch.

BTW, I agree that patches should be improving code quality but I wanted to highlight that these changes were actually based on the current ARM64 implementation. E.g: this snip and this requested change. But thanks for pointing it out.

source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h
98–99 ↗(On Diff #118942)

It's a native only compiled file (see line 13. It defines a #if defined(__powerpc64__) that ends at the end of file)

In D38897#897378, @gut wrote:

Thanks for supporting this change. I guess @anajuliapc will add you both as reviewer as soon as she updates this patch.

BTW, I agree that patches should be improving code quality but I wanted to highlight that these changes were actually based on the current ARM64 implementation. E.g: this snip and this requested change. But thanks for pointing it out.

I'd be equally fine with extracting that logic into a separate function, it looks mostly the same? I don't think we should be duplicating code (or even algorithms written slightly differently)

anajuliapc added inline comments.Oct 16 2017, 3:17 AM
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp
374 ↗(On Diff #118942)

Yes, It's strange but is the only way it works without changing NativeProcessLinux::PtraceWrapper. PPC_PTRACE_DELHWDEBUG works different from other PPC ptrace requests, because it must pass the return of PPC_PTRACE_SETHWDEBUG to ptrace instead of a data structure. If you pass it as long, PtraceWrapper fails because it must receive a void*.

I'll check the other points, thanks for the comments.

anajuliapc marked 6 inline comments as done.
  • Add values to header file
  • Use std::array to declare m_hwp_regs
  • Use llvm's mask
  • Remove switch and use assert instead
  • Remove unnecessary 'else'
  • Change return

I applied some of the suggestions. I'm still working on the others, since I may need to change some logic

Thanks for the comments

anajuliapc marked 2 inline comments as done.
  • Change numeric values to enum
  • Use llvm's functions to align addresses
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp
327–333 ↗(On Diff #118942)

I don't think it's worth the effort to do it like that. This function returns wp_index anyway, and it's easier to limit the number of watchpoints to PowerPC's actual limit, which is given by ptrace.

416–417 ↗(On Diff #118942)

No because it's called by NativeProcessLinux., I would have to change it too and I don't think I should since is a common file.

source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h
70–72 ↗(On Diff #118942)

I had some ideas while implementing your sugestions... I'm thinking about removing these functions and put these information in m_hwp_regs struct. Do you think I should do it in a new patch?

clayborg added inline comments.Oct 20 2017, 8:09 AM
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp
284 ↗(On Diff #119567)

We should use the lldb::WatchpointKind from lldb-enumerations.h:

//----------------------------------------------------------------------
// Watchpoint Kind
// Indicates what types of events cause the watchpoint to fire.
// Used by Native*Protocol-related classes.
//----------------------------------------------------------------------
FLAGS_ENUM(WatchpointKind){eWatchpointKindRead = (1u << 0),
                           eWatchpointKindWrite = (1u << 1)};

The you look at the bits and do the right thing. "access_mode" used below will just be if read and write are set.

anajuliapc added inline comments.Oct 20 2017, 12:18 PM
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp
284 ↗(On Diff #119567)

Variable watch_flags is set in GDBRemoteCommunicationServerLLGS with read = 2 and write = 1.
Where do you think I should change it?

clayborg added inline comments.Oct 20 2017, 1:07 PM
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp
284 ↗(On Diff #119567)

I would switch them over to use eWatchpointKindRead and eWatchpointKindWrite and modify all code that falls out from that to use these definitions as well.

  • Switch over read and write eWatchpointKind to match with watch_flags
anajuliapc marked 2 inline comments as done.Oct 23 2017, 4:57 AM
gut added inline comments.Oct 23 2017, 6:22 AM
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp
292 ↗(On Diff #119835)

As these enums are bitmasks, the traditional way of checking if more than one is set is to use the bitwise OR operator (|). Mostly because of the semantics, as you're not doing arithmetic there.

anajuliapc marked an inline comment as done.
  • Use bitwise OR operator
clayborg accepted this revision.Oct 23 2017, 8:00 AM

Looks fine. Thanks for doing the changes.

This revision is now accepted and ready to land.Oct 23 2017, 8:00 AM
anajuliapc added inline comments.Oct 23 2017, 8:09 AM
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp
59 ↗(On Diff #119847)

I just realized I forgot to remove the enum I've created before. I know you already accepted this patch, but may I update it anyway?

  • Remove unused enum

Yeah, I meant to suggest to remove that. Remove and submit as needed. Thanks again.

gut added a comment.Oct 26 2017, 11:30 AM

Looks fine. Thanks for doing the changes.

Hi, it's been already 4 days since this patch was accepted but not merged. Did something happen internally or we just need to wait a little longer?

gut added a subscriber: eugene.Oct 27 2017, 7:54 AM
In D38897#908283, @gut wrote:

Looks fine. Thanks for doing the changes.

Hi, it's been already 4 days since this patch was accepted but not merged. Did something happen internally or we just need to wait a little longer?

@eugene : Hi, I wonder if you can commit this accepted review for us. The other 2 reviews from our teams were pushed by you (D36804 D38323)

Thanks in advance.

This revision was automatically updated to reflect the committed changes.