[PATCH] D48446: [ubsan] Add support for reporting diagnostics to a monitor process

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka accepted this revision.
vitalybuka added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/ubsan/ubsan_monitor.h:36
+/// that a UB report is available.
+extern "C" SANITIZER_INTERFACE_ATTRIBUTE void __ubsan_on_report(void);
+
----------------
vsk wrote:
> vitalybuka wrote:
> > kubamracek wrote:
> > > vitalybuka wrote:
> > > > vsk wrote:
> > > > > vitalybuka wrote:
> > > > > > vsk wrote:
> > > > > > > vitalybuka wrote:
> > > > > > > > I'd prefer interface consistent with similar one from TSAN:
> > > > > > > > __tsan_on_report(void *report) {
> > > > > > > >   __tsan_get_report_loc(report.....
> > > > > > > >   __tsan_get_report_loc_object_type(report.....
> > > > > > > > }
> > > > > > > > 
> > > > > > > > Also this will avoid having state set by RegisterUndefinedBehaviorReport
> > > > > > > Point taken, it would be a bit simpler this way. However, we can't make this change because it would break debuggability of code compiled with Xcode 9.
> > > > > > Does Xcode 9 works for tsan?
> > > > > Are you asking if IDE diagnostics for tsan issues found in a binary compiled with Xcode 9, work when running under Xcode (9+K)? I'm not sure. Generally we try to make it possible to debug old binaries.
> > > > I am confused. Could you please elaborate why change in non existing yet API will break code compiled with Xcode 9?
> > > > 
> > > > To clarify, I propose to have:
> > > > ```
> > > > __ubsan_on_report(void *report) {
> > > >     __ubsan_get_report_loc(report.....
> > > >     __ubsan_get_report_other_stuff(report.....
> > > > }
> > > > ...
> > > > similar to tsan
> > > Yes, Xcode 9 supports TSan and UBSan. I think we really need to keep the interface as it is, at least for this initial commit. Changes in the interface should then try to be as compatible as possible. Every time TSan's debugging interface changed, we did that in a backwards-compatible manner (debugging old binaries works), and forwards-compatible way as well (worst case is you'll miss some data in the report).
> > Not sure I understand the answer.
> > Does Xcode 9 already expects __ubsan_on_report and __ubsan_get_current_report_data as is?
> Yes, I'd like to upstream the compiler-rt support. One benefit to the community is that downloadable open-source toolchains will become compatible with Xcode's ubsan support.
Thanks. I see now.


https://reviews.llvm.org/D48446