[PATCH] D35864: [sanitizer_common] Fuchsia port

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab requested changes to this revision.
filcab added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/sanitizer_common/sanitizer_common_libcdep.cc:43
+
+static INLINE bool ReportSupportsColors() {
+  return true;
----------------
In Fuchsia the output will never go into a file or somewhere where colors aren't supported?


================
Comment at: lib/sanitizer_common/sanitizer_file.cc:20
 
+#if !SANITIZER_FUCHSIA
+
----------------
Usual nit: `sanitizer_platform.h` -> Test for `SANITIZER_FUCHSIA`.


================
Comment at: lib/sanitizer_common/sanitizer_fuchsia.cc:16
+#include "sanitizer_fuchsia.h"
+#if SANITIZER_FUCHSIA
+
----------------
Assuming `sanitizer_fuchsia.h` has a proper guard, I'm ok with this as is, not too far from including `sanitizer_platform.h` only.


================
Comment at: lib/sanitizer_common/sanitizer_fuchsia.h:16
+
+#include "sanitizer_common.h"
+#if SANITIZER_FUCHSIA
----------------
Avoid parsing useless includes by always including `sanitizer_platform.h` first and testing for the platform you want.


================
Comment at: lib/sanitizer_common/sanitizer_platform.h:279
 // that this allocation happens in dynamic linker and should be ignored.
-#if SANITIZER_PPC || defined(__thumb__)
+#if (SANITIZER_LINUX && !SANITIZER_ANDROID) && \
+  (SANITIZER_PPC || defined(__thumb__))
----------------
Doesn't seem Fuchsia related.


================
Comment at: lib/sanitizer_common/sanitizer_platform_interceptors.h:25
+
+// TODO(mcgrathr): Should be renamed SI_UNIX or something.
+#if SANITIZER_WINDOWS || SANITIZER_FUCHSIA
----------------
`NOT_WINDOWS` doesn't mean UNIX, though. We do have `SI_POSIX`, which might be close to what you need?


================
Comment at: lib/sanitizer_common/sanitizer_platform_interceptors.h:26
+// TODO(mcgrathr): Should be renamed SI_UNIX or something.
+#if SANITIZER_WINDOWS || SANITIZER_FUCHSIA
 # define SI_NOT_WINDOWS 0
----------------
This seems highly misleading.


================
Comment at: lib/sanitizer_common/sanitizer_platform_interceptors.h:33
+#if SI_NOT_WINDOWS
+# include "sanitizer_platform_limits_posix.h"
 #endif
----------------
Why not `#if SI_POSIX`? Or maybe add some additional OS there.


================
Comment at: lib/sanitizer_common/sanitizer_platform_interceptors.h:120
-#define SANITIZER_INTERCEPT_MEMCHR 1
-#define SANITIZER_INTERCEPT_MEMRCHR (SI_FREEBSD || SI_LINUX || SI_NETBSD)
 
----------------
Where did this go?


================
Comment at: lib/sanitizer_common/sanitizer_stacktrace_printer.cc:36
+  // Fuchsia uses its own bespoke format to enable offline symbolization.
+  RenderStackFrame(buffer, frame_no, info.address);
+#else
----------------
Maybe have a different default? Or are you planning on simply not having this type of functionality (allow users to set their stacktrace format)?


================
Comment at: lib/sanitizer_common/sanitizer_stacktrace_printer.cc:131
+  // Fuchsia uses its own bespoke format to enable offline symbolization.
+  RenderDataInfo(buffer, DI);
+#else
----------------
Same as above.


================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc:17
 #include "sanitizer_symbolizer_internal.h"
+#if !SANITIZER_FUCHSIA
 
----------------
Usual comment. Don't parse headers you don't need.


Repository:
  rL LLVM

https://reviews.llvm.org/D35864