[Lldb-commits] [PATCH] D11936: Download symbol file for .oat files on android

[Oleksiy Vyalov via lldb-commits]

ovyalov added a comment.

Please see my comments.


================
Comment at: source/Plugins/Platform/Android/PlatformAndroid.cpp:306
@@ +305,3 @@
+    if (!module_sp->GetPlatformFileSpec())
+    return Error("No platform file specified");
+    
----------------
indentation.

================
Comment at: source/Plugins/Platform/Android/PlatformAndroid.cpp:325
@@ +324,3 @@
+                                  &tmpdir,
+                                  1);
+
----------------
Please add constant for timeout.

================
Comment at: source/Plugins/Platform/Android/PlatformAndroid.cpp:329
@@ +328,3 @@
+        return Error("Failed to generate temporary directory on the device (%s)", error.AsCString());
+    tmpdir.erase(tmpdir.size() - 1); // Remove trailing new line
+
----------------
Please check that tmpdir isn't empty.

================
Comment at: source/Plugins/Platform/Android/PlatformAndroid.cpp:331
@@ +330,3 @@
+
+    // Create file remover for the temporary directroy created on the device
+    std::unique_ptr<std::string, std::function<void(std::string*)>> tmpdir_remover(
----------------
s/directroy/directory

================
Comment at: source/Plugins/Platform/Android/PlatformAndroid.cpp:337
@@ +336,3 @@
+            command.Printf("rm -rf %s", s->c_str());
+            this->RunShellCommand(command.GetData(),
+                                  GetWorkingDirectory(),
----------------
Please log an error if rm fails.

================
Comment at: source/Plugins/Platform/Android/PlatformAndroid.cpp:342
@@ +341,3 @@
+                                  nullptr,
+                                  1);
+        }
----------------
Can we have a bigger timeout? 1 sec might not be enough.

================
Comment at: source/Plugins/Platform/Android/PlatformAndroid.cpp:352
@@ +351,3 @@
+    command.Printf("oatdump --symbolize=%s --output=%s",
+                   module_sp->GetPlatformFileSpec().GetCString(),
+                   symfile_platform_filespec.GetCString());
----------------
GetCString(false)?

================
Comment at: source/Plugins/Platform/Android/PlatformAndroid.cpp:353
@@ +352,3 @@
+                   module_sp->GetPlatformFileSpec().GetCString(),
+                   symfile_platform_filespec.GetCString());
+    error = RunShellCommand(command.GetData(),
----------------
ditto - please verify that it works as expected from Windows.

================
Comment at: source/Utility/ModuleCache.cpp:137
@@ +136,3 @@
+
+    FileSpec symfile_spec((cached_module_sp->GetFileSpec ().GetPath () + ".sym").c_str (), false);
+    if (symfile_spec.Exists ())
----------------
Please add ".sym" constant

================
Comment at: source/Utility/ModuleCache.cpp:197
@@ +196,3 @@
+
+    // Fetching a symbol file for the modul
+    const auto tmp_download_sym_file_spec = JoinPath (module_spec_dir, kTempSymFileName);
----------------
s/modul/module

================
Comment at: source/Utility/ModuleCache.cpp:207
@@ +206,3 @@
+    FileSpec symfile_spec((cached_module_sp->GetFileSpec ().GetPath () + ".sym").c_str (), false);
+    error = Put (root_dir_spec, hostname, module_spec, tmp_download_sym_file_spec, symfile_spec);
+    if (error.Fail ())
----------------
Do we really want to have a link for sym file in sys root? 
I'd rather moved code for sym file download from GetAndPut into Put method and skip link creation for symbol file.


http://reviews.llvm.org/D11936