[PATCH] D36810: Minimal runtime for UBSan.

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis added inline comments.


================
Comment at: compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cc:29
+
+    if (sz == kMaxCallerPcs) message("ubsan: too many errors\n");
+    if (sz >= kMaxCallerPcs) return false;
----------------
vitalybuka wrote:
> Could this be at line 20
> if (sz == kMaxCallerPcs - 1) message("ubsan: too many errors\n");
>  if (sz >= kMaxCallerPcs - 1) return false;
No, that way "too many errors" message may be printed multiple times.

Added one more check to the loop above: if we read a nullptr value from caller_pcs[i], it means that another thread has bumped caller_pcs_sz but has not written the pc value into the new element yet. 


================
Comment at: compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cc:53
+    if (!report_this_error(__builtin_return_address(0))) return; \
+    message("ubsan: " msg "\n");                                 \
+  }                                                              \
----------------
vsk wrote:
> eugenis wrote:
> > vsk wrote:
> > > Printing out the return address here would aid debugging. It's extra complexity but would be worth it when triaging bugs, or when using the minimal runtime in low-level software.
> > Even if it's just an absolute address, w/o module base? It's does not give you much in the presence of ASLR.
> > 
> This would still be helpful while debugging xnu, or firmware which runs without ASLR. While debugging the kernel you can print out the slide offset for the text segment.
I've added a FIXME for now.


================
Comment at: compiler-rt/test/ubsan_minimal/lit.common.cfg:32
+# Check that the host supports UndefinedBehaviorSanitizerMinimal tests
+if config.host_os not in ['Linux', 'FreeBSD', 'NetBSD']: # TODO: 'Darwin', 'Windows'
+  config.unsupported = True
----------------
vsk wrote:
> We should be able to just include Darwin here too.
Would you like me to add Darwin to COMPILER_RT_HAS_UBSAN_MINIMAL in config-ix.cmake, too? I don't have a machine to test it though.


https://reviews.llvm.org/D36810