[Lldb-commits] [PATCH] D45298: [debugserver] Fix LC_BUILD_VERSION load command handling.

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda accepted this revision.
jasonmolenda added a comment.
This revision is now accepted and ready to land.

Looks good to me.



================
Comment at: packages/Python/lldbsuite/test/decorators.py:359
+            output = subprocess.check_output(["xcodebuild", "-showsdks"], stderr=DEVNULL)
+            if re.search('%ssimulator' % platform, output):
+                return None
----------------
It might be helpful for anyone debugging this if examples of the names this is matching were included in a comment.  iphonesimulator, appletvsimulator, watchsimulator.  Different capitalization is used in so many places (and whether to include "os" suffix or not, and whether to include "apple" prefix or not) that it can be hard to know what's expected unless you run the command yourself.


================
Comment at: tools/debugserver/source/MacOSX/MachProcess.mm:584
+    cmd == LC_VERSION_MIN_IPHONEOS || cmd == LC_VERSION_MIN_MACOSX;
+#if defined(LC_VERSION_MIN_TVOS)
+  lc_cmd_known |= cmd == LC_VERSION_MIN_TVOS;
----------------
Not important, but we're requiring a MacOS SDK of 10.11 or newer and the tvos and watchos SDKs were available by then; we could remove these ifdefs.  (LC_BUILD_VERSION is newer than that & needs to be kept around.)


================
Comment at: tools/debugserver/source/MacOSX/MachProcess.mm:588
+#if defined(LC_VERSION_MIN_WATCHOS)
+  lc_cmd_known |= cmd == LC_VERSION_MIN_WATCHOS;
+#endif
----------------
It's equivalent, no big deal, but this is a bitwise | not a logical ||.


https://reviews.llvm.org/D45298