sgraenitz added inline comments.

================
Comment at: tools/debugserver/source/CMakeLists.txt:101
+option(LLDB_NO_DEBUGSERVER "Delete debugserver after building it, and don't 
try to codesign it" OFF)
+option(LLDB_USE_SYSTEM_DEBUGSERVER "Neither build nor codesign debugserver. 
Use the system's debugserver instead (Darwin only)." OFF)
+
----------------
JDevlieghere wrote:
> Should we emit and error if these variables are in an inconsistent state, 
> e.g. when both are set? 
Yes, could do that.


================
Comment at: tools/debugserver/source/CMakeLists.txt:118
+      OUTPUT_VARIABLE xcode_dev_dir)
+    string(STRIP ${xcode_dev_dir} xcode_dev_dir)
+
----------------
JDevlieghere wrote:
> Why did you make this variable name lowercase? Does that have any semantic 
> meaning?
LLVM tends to use lowercase names for local/non-cached variables, e.g. 
`update_src_props` in:
https://github.com/llvm-mirror/llvm/blob/master/cmake/modules/AddLLVM.cmake

I didn't find any guidelines on conventions for CMake, but I thought it's a 
good idea. It's a little surprising to mix in such changes to patches that also 
have semantic changes. Maybe I should continue to use uppercase names for now 
and add a subsequent patch (with a proper title) that does the change for the 
whole file. What do you think?


================
Comment at: tools/debugserver/source/CMakeLists.txt:128-133
+    # TODO: Following the old behavior, DEBUGSERVER_PATH still points to the
+    # original system binary, even if we copy it over. Keep this?
+    set(DEBUGSERVER_PATH 
"${lldb_framework_dir}/LLDB.framework/Resources/debugserver" CACHE FILEPATH "" 
FORCE)
+
+    # If we haven't built a signed debugserver. If possible, copy the one from
+    # the system to make the built debugger functional on Darwin.
----------------
xiaobai wrote:
> I'm pretty sure that if you do not copy debugserver it will still find the 
> one from your `${lldb_framework_dir}` when you run lldb anyway. Do you know 
> why we copy it over?
Yes this is what I mean with "old behaviour" here. `DEBUGSERVER_PATH` still 
points to the original system binary and gets passed to tests, e.g. in 
`test/CMakeLists.txt` above.


https://reviews.llvm.org/D54476



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to