[PATCH] D11791: [Windows] Use llvm-symbolizer before using dbghelp

[Alexey Samsonov via llvm-commits]

samsonov added inline comments.

================
Comment at: CMakeLists.txt:358
@@ -357,1 +357,3 @@
 
+set(COMPILER_RT_LLD_PATH ${LLVM_MAIN_SRC_DIR}/tools/lld)
+if(EXISTS ${COMPILER_RT_LLD_PATH}/)
----------------
Please land this (together with lit config and `test/cfi` changes) in a separate small commit.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc:306
@@ +305,3 @@
+
+  // If we got a function name, we probably had debug info.
+  return last->info.function != nullptr;
----------------
Why do you need to optionally return false (and fallback to the next symbolizer in chain here?

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc:401
@@ +400,3 @@
+    // its stdout.
+    if (!success || just_read == 0 || just_read == (uptr)-1) {
+      Report("WARNING: Can't read from symbolizer at fd %d (error %d)\n",
----------------
Looks like `just_read` can't be -1 now, when we properly return the error from `ReadFromFile` function. Just delete this case.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc:420
@@ +419,3 @@
+  bool success = WriteToFile(output_fd_, buffer, length, &write_len, &err);
+  if (!success || write_len == 0 || write_len == (uptr)-1) {
+    Report("WARNING: Can't write to symbolizer at fd %d (error %d)\n",
----------------
Same here: `write_len` can't be -1 if `success` is true. In fact, these changes (and `CloseFile` change above) are bugfixes, because checking for -1 was wrong in the first place - one should have called `internal_iserror` on the result of `internal_write` instead. I would appreciate if you will replace internal_read / internal_write / internal_close with ReadFromFile / WriteToFile / CloseFile as a separate small commit preceding this one.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_process_libcdep.cc:1
@@ -1,2 +1,2 @@
 //===-- sanitizer_symbolizer_process_libcdep.cc ---------------------------===//
 //
----------------
This file makes little sense now, after you've moved significant part of `SymbolizerProcess` implementation to `sanitizer_symbolizer_libcdep.cc`. Probably, now you can move the only remaining function to `sanitizer_symbolizer_posix_libcdep.cc`, and delete this file?


http://reviews.llvm.org/D11791