This is an archive of the discontinued LLVM Phabricator instance.

[asan] Move exception code to sanitizer_common.
ClosedPublic

Authored by mpividori on Feb 2 2017, 9:26 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

mpividori created this revision.Feb 2 2017, 9:26 AM
rnk added a reviewer: etienneb.Feb 2 2017, 9:31 AM
etienneb edited edge metadata.Feb 2 2017, 9:38 AM

This code will be used by an other sanitizer?

lib/asan/asan_win.cc
239 ↗(On Diff #86831)

You should move the comment too.

rnk accepted this revision.Feb 2 2017, 9:41 AM

lgtm

This revision is now accepted and ready to land.Feb 2 2017, 9:41 AM

This code will be used by an other sanitizer?

Hi @etienneb,
We want to have general support for Windows, not only on asan. Because of that all the refactory in:

lib/asan/asan_win.cc
239 ↗(On Diff #86831)

You are right.

kcc added a subscriber: kcc.Feb 2 2017, 10:33 AM
kcc added inline comments.
lib/sanitizer_common/sanitizer_common.h
384 ↗(On Diff #86831)

no #if please.
The only #if we accept (except for some special files) is a single #if around the entire file.

rnk added inline comments.Feb 2 2017, 10:43 AM
lib/sanitizer_common/sanitizer_common.h
384 ↗(On Diff #86831)

sanitizer_common.h has a lot of #ifs, so I thought this was acceptable. If we just remove the if here, a caller will get a link error. Would you prefer that?

mpividori added inline comments.Feb 2 2017, 10:53 AM
lib/sanitizer_common/sanitizer_common.h
384 ↗(On Diff #86831)

@kcc if you prefer I can add an empty implementation for Posix.

kcc added a comment.Feb 2 2017, 11:11 AM

The amount if #if statements is extremely annoying, but we clearly don't want any more.

I am not sure why you need this function declaration in lib/sanitizer_common/sanitizer_common.h
given that it's definition and its use is in *win* files.

In D29458#665040, @kcc wrote:

The amount if #if statements is extremely annoying, but we clearly don't want any more.

I am not sure why you need this function declaration in lib/sanitizer_common/sanitizer_common.h
given that it's definition and its use is in *win* files.

Because we don't have a specific header for Windows interface in sanitizer_common. If you prefer, I can create: sanitizer_windows.h and add it there.

kcc added a comment.Feb 2 2017, 11:32 AM

yes.
maybe, sanitizer_win.h

mpividori updated this revision to Diff 86882.Feb 2 2017, 2:24 PM

@kcc I added a new header. Let me know if you agree.
Thanks!

kcc accepted this revision.Feb 2 2017, 2:30 PM

LGTM

This revision was automatically updated to reflect the committed changes.