This is an archive of the discontinued LLVM Phabricator instance.

Differential D71772

[scudo][standalone] Support __BIONIC__
ClosedPublic

Authored by cryptoad on Dec 20 2019, 8:24 AM.

Details

Reviewers
cferris
hctim
pcc
eugenis
morehouse
Commits
rG0fd6f19025a7: [scudo][standalone] Support __BIONIC__
Summary

Some Android builds that we are interested in define __BIONIC__
but not __ANDROID__, so expand SCUDO_ANDROID to encompass those.

Diff Detail

Event Timeline

cryptoad created this revision.Dec 20 2019, 8:24 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 20 2019, 8:24 AM
Herald added subscribers: Restricted Project, krytarowski. · View Herald Transcript
Harbormaster completed remote builds in B42834: Diff 234906.Dec 20 2019, 8:29 AM
pcc added inline comments.Dec 20 2019, 9:16 AM
compiler-rt/lib/scudo/standalone/platform.h
21–22

Maybe just #if defined(__BIONIC__)?

cryptoad added inline comments.Dec 20 2019, 9:26 AM
compiler-rt/lib/scudo/standalone/platform.h
21–22

We still want Scudo to work on Android outside of Bionic (eg: .so or just statically linked to a binary) in which case I don't think BIONIC will be defined.

cferris added inline comments.Dec 20 2019, 9:43 AM
compiler-rt/lib/scudo/standalone/platform.h
21–22

It's also the case that BIONIC is only defined when you include cdefs.h directly or indirectly. Unfortunately, at this point it's not defined so this doesn't quite solve the problem.

pcc added inline comments.Dec 20 2019, 9:43 AM
compiler-rt/lib/scudo/standalone/platform.h
21–22

I don't think that __BIONIC__ means that we're compiling as part of Bionic, it just means that the libc is Bionic. See:
https://android.googlesource.com/platform/bionic/+/master/docs/defines.md

To confuse matters, we also have _BIONIC, which, at least within scudo, does mean that we're compiling as part of Bionic. It may be a good idea to rename this macro to something more clear.

cryptoad marked an inline comment as done.Dec 20 2019, 9:59 AM
cryptoad added inline comments.
compiler-rt/lib/scudo/standalone/platform.h
21–22

Right now there is no #include in platform.h which I feel is what it should be.
Should I include cdefs.h and do a defined(__BIONIC__) or work around it?

cryptoad marked an inline comment as not done.Dec 20 2019, 9:59 AM
cryptoad added inline comments.Dec 20 2019, 10:12 AM
compiler-rt/lib/scudo/standalone/platform.h
21–22

(also the initial patch won't work)

pcc added inline comments.Dec 20 2019, 10:13 AM
compiler-rt/lib/scudo/standalone/platform.h
21–22

I'm not sure that you can include cdefs.h because it isn't universally available, e.g. the Windows and Fuchsia SDKs don't appear to have it.

One option would be to include a standardized header such as stdint.h which does include cdefs.h on Android.

cryptoad updated this revision to Diff 234923.Dec 20 2019, 10:37 AM

New proposal as per Peter's suggestion: include stdint.h in
platform.h and check for __BIONIC__.

Harbormaster completed remote builds in B42844: Diff 234923.Dec 20 2019, 10:40 AM
pcc accepted this revision.Dec 20 2019, 10:43 AM

LGTM

This revision is now accepted and ready to land.Dec 20 2019, 10:43 AM
cferris added a comment.Dec 20 2019, 11:11 AM

I verified this fixes the case of building bionic on linux, and it also builds properly in the normal android build.

compiler-rt/lib/scudo/standalone/platform.h
12

Transitive.

cryptoad updated this revision to Diff 234933.Dec 20 2019, 11:19 AM

Correcting comment typo.

Harbormaster completed remote builds in B42849: Diff 234933.Dec 20 2019, 11:21 AM
cryptoad marked 8 inline comments as done.Dec 20 2019, 11:23 AM
cferris accepted this revision.Dec 20 2019, 12:48 PM

LGTM

Closed by commit rG0fd6f19025a7: [scudo][standalone] Support __BIONIC__ (authored by cryptoad). · Explain WhyDec 20 2019, 1:04 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
compiler-rt/
lib/
scudo/
standalone/
6 lines

Diff 234923

compiler-rt/lib/scudo/standalone/platform.h

//===-- platform.h ----------------------------------------------*- C++ -*-===// //===-- platform.h ----------------------------------------------*- C++ -*-===//
// //
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information. // See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#ifndef SCUDO_PLATFORM_H_ #ifndef SCUDO_PLATFORM_H_
#define SCUDO_PLATFORM_H_ #define SCUDO_PLATFORM_H_
// Transtive includes of stdint.h specify some of the defines checked below.
cferrisUnsubmitted
Done
ReplyInline Actions

Transitive.

cferris: Transitive.
#include <stdint.h>
#if defined(__linux__) #if defined(__linux__)
#define SCUDO_LINUX 1 #define SCUDO_LINUX 1
#else #else
#define SCUDO_LINUX 0 #define SCUDO_LINUX 0
#endif #endif
#if defined(__ANDROID__) // See https://android.googlesource.com/platform/bionic/+/master/docs/defines.md
#if defined(__BIONIC__)
pccUnsubmitted
Done
ReplyInline Actions

Maybe just #if defined(__BIONIC__)?

pcc: Maybe just `#if defined(__BIONIC__)`?
cryptoadAuthorUnsubmitted
Done
ReplyInline Actions

We still want Scudo to work on Android outside of Bionic (eg: .so or just statically linked to a binary) in which case I don't think BIONIC will be defined.

cryptoad: We still want Scudo to work on Android outside of Bionic (eg: .so or just statically linked to…
pccUnsubmitted
Done
ReplyInline Actions

I don't think that __BIONIC__ means that we're compiling as part of Bionic, it just means that the libc is Bionic. See:
https://android.googlesource.com/platform/bionic/+/master/docs/defines.md

To confuse matters, we also have _BIONIC, which, at least within scudo, does mean that we're compiling as part of Bionic. It may be a good idea to rename this macro to something more clear.

pcc: I don't think that `__BIONIC__` means that we're compiling as part of Bionic, it just means…
cferrisUnsubmitted
Done
ReplyInline Actions

It's also the case that BIONIC is only defined when you include cdefs.h directly or indirectly. Unfortunately, at this point it's not defined so this doesn't quite solve the problem.

cferris: It's also the case that __BIONIC__ is only defined when you include cdefs.h directly or…
cryptoadAuthorUnsubmitted
Done
ReplyInline Actions

Right now there is no #include in platform.h which I feel is what it should be.
Should I include cdefs.h and do a defined(__BIONIC__) or work around it?

cryptoad: Right now there is no `#include` in platform.h which I feel is what it should be. Should I…
pccUnsubmitted
Done
ReplyInline Actions

I'm not sure that you can include cdefs.h because it isn't universally available, e.g. the Windows and Fuchsia SDKs don't appear to have it.

One option would be to include a standardized header such as stdint.h which does include cdefs.h on Android.

pcc: I'm not sure that you can include cdefs.h because it isn't universally available, e.g. the…
cryptoadAuthorUnsubmitted
Done
ReplyInline Actions

(also the initial patch won't work)

cryptoad: (also the initial patch won't work)
#define SCUDO_ANDROID 1 #define SCUDO_ANDROID 1
#else #else
#define SCUDO_ANDROID 0 #define SCUDO_ANDROID 0
#endif #endif
#if defined(__Fuchsia__) #if defined(__Fuchsia__)
#define SCUDO_FUCHSIA 1 #define SCUDO_FUCHSIA 1
#else #else
▲ Show 20 Lines • Show All 44 Lines • Show Last 20 Lines