[Lldb-commits] [PATCH] D13812: Increase default memory cache line size for android

[Greg Clayton via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

I would rather you be able to ask the current platform for the default memory cache line size and have lldb_private::Process do this automatically on the first memory read if the process property wasn't manually set. So my inline comments will reflect this notion.


================
Comment at: include/lldb/Target/Process.h:70-72
@@ -69,2 +69,5 @@
 
+    lldb::OptionValueSP
+    GetMemoryCacheLineSizeOption ();
+
     Args
----------------
Remove

================
Comment at: source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp:148
@@ +147,3 @@
+    lldb::ProcessSP process_sp = PlatformRemoteGDBServer::DebugProcess(launch_info, debugger, target, error);
+    AdjustProcessProperties(process_sp);
+    return process_sp;
----------------
Remove the call to AdjustProcessProperties(...) and let the process call platform_sp->GetDefaultMemoryCacheLineSize() when it needs to. If the only this this PlatformAndroidRemoteGDBServer::DebugProcess() was doing was to allow calling AdjustProcessProperties() then remove this whole function.

================
Comment at: source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp:159
@@ +158,3 @@
+    lldb::ProcessSP process_sp = PlatformRemoteGDBServer::Attach(attach_info, debugger, target, error);
+    AdjustProcessProperties(process_sp);
+    return process_sp;
----------------
Remove the call to AdjustProcessProperties(...) and let the process call platform_sp->GetDefaultMemoryCacheLineSize() when it needs to. If the only this this PlatformAndroidRemoteGDBServer::Attach() was doing was to allow calling AdjustProcessProperties() then remove this whole function.

================
Comment at: source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp:163-174
@@ -139,1 +162,14 @@
+
+void
+PlatformAndroidRemoteGDBServer::AdjustProcessProperties(const lldb::ProcessSP &process_sp)
+{
+    if (! process_sp)
+        return;
+
+    lldb::OptionValueSP value_sp = process_sp->GetMemoryCacheLineSizeOption();
+
+    if (value_sp && ! value_sp->OptionWasSet())
+        value_sp->SetUInt64Value(g_android_default_cache_size);
+}
+
 void
----------------
Change this to be:
```
uint32_t
PlatformAndroidRemoteGDBServer:: GetDefaultMemoryCacheLineSize()
{
    return g_android_default_cache_size;
}
```

Add the following to Platform.h inside the lldb_private::Platform class definition:

```
virtual uint32_t GetDefaultMemoryCacheLineSize() { return 0; }
```


================
Comment at: source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h:39-49
@@ -38,1 +38,13 @@
 
+    lldb::ProcessSP
+    DebugProcess(ProcessLaunchInfo &launch_info,
+                 Debugger &debugger,
+                 Target *target,       // Can be NULL, if NULL create a new target, else use existing one
+                 Error &error) override;
+
+    lldb::ProcessSP
+    Attach(ProcessAttachInfo &attach_info,
+           Debugger &debugger,
+           Target *target,       // Can be NULL, if NULL create a new target, else use existing one
+           Error &error) override;
+
----------------
If the only this these fucntions were added for was to allow calling AdjustProcessProperties() then remove these.

================
Comment at: source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h:55-56
@@ -42,1 +54,4 @@
 
+    void
+    AdjustProcessProperties(const lldb::ProcessSP &process_sp);
+
----------------
Change to be:

```
uint32_t GetDefaultMemoryCacheLineSize() override;
```

================
Comment at: source/Target/Process.cpp:183-189
@@ -182,1 +182,9 @@
 
+lldb::OptionValueSP
+ProcessProperties::GetMemoryCacheLineSizeOption()
+{
+    const uint32_t idx = ePropertyMemCacheLineSize;
+    lldb_private::ExecutionContext exe_ctx(m_process);
+    return m_collection_sp->GetPropertyAtIndex(&exe_ctx, true, idx)->GetValue();
+}
+
----------------
Remove this and modify Process to fetch this information on attach if the process option wasn't set. This means you don't need to expose this OptionValueSP and you can do it all within the process by grabbing the platform from the target and then calling platform_sp->GetDefaultMemoryCacheLineSize(). The default platform implementation should return 0 and if zero is returned (meaning no preference), then continue using the current setting that process was using from the setting, else override the current value. 




http://reviews.llvm.org/D13812