Page MenuHomePhabricator

[NFC] Add a test for the inferior memory cache (mainly L1)
AbandonedPublic

Authored by jarin on Apr 9 2020, 4:27 AM.

Details

Reviewers
labath
Summary

This patch adds a test for L1 of the inferior's memory cache and makes the cache testable. This is mostly in preparation for an L1 flushing bug fix and its regression test (context: https://reviews.llvm.org/D77765).

Diff Detail

Event Timeline

jarin created this revision.Apr 9 2020, 4:27 AM
labath added a comment.Apr 9 2020, 5:06 AM

Thanks for splitting this up. This looks fine to me, modulo some nits, but lets wait @clayborg has to say.

lldb/source/Target/Memory.cpp
63

I guess this is a leftover from splitting the patches?

Speaking of post-increment the llvm rule is to use pre-increment whereever possible. I see the test uses post-increment exclusively for no good reason.

lldb/unittests/Target/MemoryCacheTest.cpp
31

You can move setting of m_inferior_read_count into the initializer.

jarin updated this revision to Diff 256257.Apr 9 2020, 5:33 AM
jarin marked 2 inline comments as done.

Addressed reviewer comments.

jarin added inline comments.Apr 9 2020, 5:34 AM
lldb/source/Target/Memory.cpp
63

But, but,... I only increment/decrement in statement position!

Just kidding, I am sorry, fixed now...

Mostly good, but it seems weird to supply the cache line size when calling the MemoryCache::Clear() function. We also don't seem to be catching updates to the cache line size property and invalidating the cache when and only when the property is modified, but we seem to be relying on calls to Clear() to do this when we stop? It would be nice to clean this up before submitting just because this change forces this now out of place looking setting to be passed along. ProcessProperties::ProcessProperties() has a place where you can hook in and call a function when the property changes:

ProcessProperties::ProcessProperties(lldb_private::Process *process)
    : Properties(),
      m_process(process) // Can be nullptr for global ProcessProperties
{
  if (process == nullptr) {
    // Global process properties, set them up one time
    m_collection_sp =
        std::make_shared<ProcessOptionValueProperties>(ConstString("process"));
    m_collection_sp->Initialize(g_process_properties);
    m_collection_sp->AppendProperty(
        ConstString("thread"), ConstString("Settings specific to threads."),
        true, Thread::GetGlobalProperties()->GetValueProperties());
  } else {
    m_collection_sp = std::make_shared<ProcessOptionValueProperties>(
        Process::GetGlobalProperties().get());
    m_collection_sp->SetValueChangedCallback(
        ePropertyPythonOSPluginPath,
        [this] { m_process->LoadOperatingSystemPlugin(true); });
  }
}

So we could add:

m_collection_sp->SetValueChangedCallback(
    ePropertyMemCacheLineSize,
    [this] { m_process->UpdateCacheLineSize(); });

This function would then update the cache line size in the m_memory_cache variable.

lldb/source/Target/Memory.cpp
31–38

Do we call clear when the cache line size changes? Maybe we should just have a function that does this and only this?:

void MemoryCache::SetCacheLineSize(uint64_t size);
lldb/source/Target/Process.cpp
614

Seems weird to be passing the cache line size to the clear method. Here we clearly just want to clear out the cache and don't care about the cache line size.

1497

Seems weird to be passing the cache line size to the clear method. Here we clearly just want to clear out the cache and don't care about the cache line size.

5615

Seems weird to be passing the cache line size to the clear method. Here we clearly just want to clear out the cache and don't care about the cache line size.

jarin added a comment.EditedApr 10 2020, 3:01 AM

Regarding the callback idea, I have bad experience with callbacks because they break if the code is not crafted for re-entrancy and they are much harder to understand. That change feels out of scope for just adding a test and fixing an unrelated bug.

Adding the SetCacheLineSize method sounds good, but if we want to keep this patch making NFC (which is what I would prefer), it would have to be called at exactly the same places as Clear. How about Pavel's idea to rename Clear -> Reset, and leave the refactoring to SetCacheLineSize for later?

It appears it is really hard to reach agreement about this, so another alternative is I submit a bug report about the L1 invalidation problem and leave it to you to figure this out. In the mean time, we will fix the bug only in our private fork of lldb. Greg, perhaps you would prefer that?

Regarding the callback idea, I have bad experience with callbacks because they break if the code is not crafted for re-entrancy and they are much harder to understand. That change feels out of scope for just adding a test and fixing an unrelated bug.

The code is already there and working. Not sure what the worry about re-entrancy is from?

Adding the SetCacheLineSize method sounds good, but if we want to keep this patch making NFC (which is what I would prefer), it would have to be called at exactly the same places as Clear. How about Pavel's idea to rename Clear -> Reset, and leave the refactoring to SetCacheLineSize for later?

We are changing the code so that just to make it NFC we have to check it in some half baked state? I don't agree with this. Clear() didn't take a parameter before, nor should it have to now.

It appears it is really hard to reach agreement about this, so another alternative is I submit a bug report about the L1 invalidation problem and leave it to you to figure this out. In the mean time, we will fix the bug only in our private fork of lldb. Greg, perhaps you would prefer that?

I don't see this as really that hard to fix correctly. Feel free to do what you need to if this is too much.

jarin added a comment.Apr 11 2020, 1:25 AM

It appears it is really hard to reach agreement about this, so another alternative is I submit a bug report about the L1 invalidation problem and leave it to you to figure this out. In the mean time, we will fix the bug only in our private fork of lldb. Greg, perhaps you would prefer that?

I don't see this as really that hard to fix correctly. Feel free to do what you need to if this is too much.

Ack, abandoning.

jarin abandoned this revision.Apr 11 2020, 1:25 AM