Page MenuHomePhabricator

[ASAN] Add support for mips/mips64 android

Authored by mohit.bhakkad on Mar 4 2016, 3:48 AM.



Continue here from D17842

Patch by Duane Sand

The mips32r2 build of asan_test runs cleanly with same result as for arm/arm64/x86: 3 failed subtests.
The mips64r6 asan_test fails early on a huge_malloc subtest without trying all tests. This is a bug in mips64 malloc, not in asan.

Using arch macros defined in D17881
some indentation changes

Diff Detail


Event Timeline

mohit.bhakkad retitled this revision from to [ASAN] Add support for mips/mips64 android.
mohit.bhakkad updated this object.
mohit.bhakkad added reviewers: kcc, eugenis, samsonov.
mohit.bhakkad set the repository for this revision to rL LLVM.
filcab added a subscriber: filcab.Mar 4 2016, 8:07 AM

I have some minor nits.

160 ↗(On Diff #49813)

Can you push the negation inside the parens, please?

1059 ↗(On Diff #49813)

Please push the negation inside the parens.

82 ↗(On Diff #49813)

I think using the FIRST_32_SECOND_64 macro would be even more readable. Even fits on one line.

samsonov added inline comments.Mar 4 2016, 11:06 AM
521 ↗(On Diff #49813)

Please don't indend #ifs

Changed indentation and some other changes.

filcab added inline comments.Mar 7 2016, 3:02 AM
595 ↗(On Diff #49935)

No need for parens.

611 ↗(On Diff #49935)

Same as line 611

Removed parens.

samsonov added inline comments.Mar 9 2016, 3:52 PM
548 ↗(On Diff #50107)

Maybe, just provide a separate definition whenever SANITIZER_ANDROID && SANITIZER_MIPS32 is true?

Changes in this revision:

  • separate definition of __sanitizer_sigactionfor mips32 android.
  • using conditional operator at sanitizer_platform_limits_posix.h:80
samsonov accepted this revision.Mar 10 2016, 10:37 AM
samsonov edited edge metadata.

LGTM with a nit. Thanks!

547 ↗(On Diff #50240)

double space before "//"

This revision is now accepted and ready to land.Mar 10 2016, 10:37 AM
duanesand added inline comments.Mar 14 2016, 12:14 PM
160 ↗(On Diff #50240)

Sorry, my mistake; this should revert back to the original condition, without SANITIZER_MIPS

162 ↗(On Diff #50240)

This #endif should be moved down around all things using this variable; either after TlsPreTcbSize, or better yet, after InitTlsSize.

173 ↗(On Diff #50240)

This function is only used when the nonempty version of InitTlsSize() is used, ie when


This TlsPreTcbSize() function must similarly be skipped when that condition is false, otherwise there is an unresolved ref to g_tls_size during mips Android builds.

189 ↗(On Diff #50240)

This condition (which must match line 159 in the original) is prone to breaking again in the future. It would be best to not duplicate it, or else give it a positive name like SANITIZER_USES_TLS.

Rather than defining it as a bunch of negatives (against what world of possible os's?), I suggest redefining it more exactly as


if that is indeed the current situation

207 ↗(On Diff #50240)

This defines a do-nothing body for InitTlsSize for some os's including Android. That allows its callers' source to be free of os dependencies, which is good. But the current way of skipping its body requires two matching instances of a non-obvious #if. These could be combined into a single #if by repeating the entire header and body of the do-nothing function in an #else part, ie

void InitTlsSize() { }

554 ↗(On Diff #50240)

final }; is missing

mohit.bhakkad edited edge metadata.

Hi @samsonov, Duane suggested some changes in sanitizer_common/, could you please review it?

mohit.bhakkad requested a review of this revision.Mar 15 2016, 3:25 AM
mohit.bhakkad edited edge metadata.
samsonov accepted this revision.Mar 15 2016, 11:54 AM
samsonov edited edge metadata.

LGTM after addressing one minor comment below. Thanks!

171 ↗(On Diff #50708)

Uhm, no, this function is only used in ThreadSelf(). Let's move it away from here, and right before ThreadSelf() definition below.

This revision is now accepted and ready to land.Mar 15 2016, 11:54 AM


Changed the location of TlsPreTcbSize() in commit.

This revision was automatically updated to reflect the committed changes.