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

[Tamas Berghammer via lldb-commits]

tberghammer added a comment.

See responds inline


================
Comment at: source/Plugins/Platform/Android/PlatformAndroid.cpp:325
@@ +324,3 @@
+                                  &tmpdir,
+                                  1);
+
----------------
ovyalov wrote:
> Please add constant for timeout.
Added a comment next to the value instead. I prefer that style but I can use a constant if you want.

================
Comment at: source/Plugins/Platform/Android/PlatformAndroid.cpp:342
@@ +341,3 @@
+                                  nullptr,
+                                  1);
+        }
----------------
ovyalov wrote:
> Can we have a bigger timeout? 1 sec might not be enough.
I think 1 second should be enough in almost every case but increase it to 5 seconds just in case the device is very busy.

================
Comment at: source/Plugins/Platform/Android/PlatformAndroid.cpp:353
@@ +352,3 @@
+                   module_sp->GetPlatformFileSpec().GetCString(),
+                   symfile_platform_filespec.GetCString());
+    error = RunShellCommand(command.GetData(),
----------------
ovyalov wrote:
> ditto - please verify that it works as expected from Windows.
At the moment I don't have Windows environment but I will try to find a way to test it.

================
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 ())
----------------
ovyalov wrote:
> 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.
At the moment I think we don't use the sys root at all, but I might be wrong. If anywhere we use the sys root then we will need the symbol file next to the actual file to find it. The reason I put the file download here because we need a Module object to do it (find out if we have symtab) what means we have to do it after the Get. I think moving it to Put will make the implementation more complicated.


http://reviews.llvm.org/D11936