yeah, lgtm from the Android side too, unless rprichard has more comments?

(yeah, similar for Android --- we'll just use the clang one as-is. but if there's already precedent for allowing include_next, that doesn't hurt us either.)

In D151585#4593279, @cferris wrote:

Sorry, I've been trying multiple ways of incorporating the value into the config and none of them work easily. They require changes to just about every file in the tree. I have a back up but that simply uses a #define instead of specialized on SCUDO_ANDROID. I think that might be the best way to go since it's not easy to use a config variable for this particular call.

I try to grep "std>" in clang/test/Headers but find nothing, and nothing in stdalign.h is about >

thanks!

i'd probably change the commit message to make it clear that Android's _global fallback_ temporary directory is /data/local/tmp. (for most apps, it's an app-specific directory that [on new enough versions of Android] you can get from $TMPDIR). but i see the _code_ works like that anyway --- prefer $TMPDIR, but fall back to /data/local/tmp. so, yes, that's correct. (i know nothing about Windows though :-) )

(sgtm from Android. we're reusing gp for SCS.)

In D150490#4343128, @hiraditya wrote:

Is there more context on why Android enables the frame pointer?

From what i gathered, this is more of an effort to have parity such that existing build flag overrides continue to be consistent.

(lgtm with craig.topper's suggested simplification.)

sgtm, but you probably want a grownup like craig.topper to do a real review :-)

thanks!

(yeah, thanks so much for doing this!)

In D147849#4256504, @rprichard wrote:

nit: In the description/commit message, the "when the API level is > 29" should be "when the API level is >= 29".

I think it's OK, but I'd like confirmation that it works with LTO?

One issue I noticed earlier was that the compiler flag (-f[no-]emulated-tls) doesn't work with LTO:

• The flag is ignored when the compiler is only outputting LLVM IR, and
• The flag is also ignored at link-time when the IR is converted to emutls-or-efltls machine code.

This link-time LTO setting can be changed using -Wl,-plugin-opt,-emulated-tls={0,1}.

thanks!

In D143619#4119248, @brad wrote:

In D143619#4115147, @luismarques wrote:

Does this mean that Android, OpenBSD, and Cygwin have emulated TLS support for all supported architectures other than RISC-V?

For OpenBSD, yes.

In D137268#4069992, @zatrazz wrote:

Could you check if this fixes your issue?

In D137268#4069856, @zatrazz wrote:

I think I have caught this because your standard conformance tests checks for gnuc_va_list
on wchar.h, wich is always defined on on GCC (git log shows it was changed to fix XPG7
tests, but I am not sure exactly why the author has changed the va_list to gnuc_va_list).

is there a corresponding glibc change so that va_list is exported for _POSIX_SOURCE cases? see https://android-review.git.corp.google.com/c/platform/bionic/+/2397313 where i'm having to disable some bionic testing against glibc because the glibc (2.17!) <wchar.h> now no longer exports va_list. i did look for a ToT glibc patch to backport (until we've _actually_ switched from glibc to musl for the host), but couldn't obviously find it?

@MaskRay for his opinion...

In D132611#3780832, @thakis wrote:

We had a bunch of gtest death tests to check that the sanitizers are working. They did a write to the left or right if an array, and checked that that a) crashed b) the crash message contained the text "left of" / "right of". Fairly easy to update, but also a bit churny, for subjectively not a lot of upside. (But if y'all think this is a better diag, that's enough of course.)

yeah, that makes sense to me.

In D128958#3653055, @hctim wrote:

For now though, to allow experimentation and further development, it would be great to temporarily reserve some section identifiers and semantics. Do you think we should use aarch64 reservations, or Android reservations? I would hope that we can avoid a RELR-style launch where Android supports some scheme for a few generations, then the generic ABI variant comes about, and then Android has to support both (and app developers have to use the old one for backwards compatibility, but that's not always obvious).

In D131057#3697392, @MaskRay wrote:

In D131057#3697095, @dim wrote:

GCC 12 -Wformat -pedantic emits a warning:

warning: ISO C17 does not support the ‘%b’ gnu_printf format [-Wformat=]

The behavior is not ported (and it's unclear to me how to implement it).

If you really want this, I think it can be implemented by looking at LangOpts::LangStd.

Something like !getLangOpts().C2X I suppose, but I do not find how to check both -Wformat and -Wpedantic.
Also, for the nature of the diagnostic, I think something in TableGen like def ext_... InGroup<C2x> would make sense but using a C23/C2x related -W option would deviate from the GCC behavior.
ISTM adding the diagnostic (even if we do) is not so necessary in this patch.

I think making scanf in the same patch makes sense. Let me check existing tests...

thanks!

In D128958#3648965, @MaskRay wrote:

The implementation looks good to me. I assume that you have authority to define a value in SHT_ANDROID_*.

In D117611#3339137, @glandium wrote:

This doesn't leave much room to use __attribute__((format(printf))) on custom printf implementations that do support %n on Android does it?

In D117611#3256097, @enh wrote:

In D117611#3256094, @enh wrote:

In D117611#3256089, @phosek wrote:

@srhines can you also take a look at the Android changes or suggest other reviewers?

sorry, took me this long to log in :-(

yeah, lgtm, thanks! %n hasn't worked for about 10 years; it changed from being silently ignored (which, in retrospect, was a cure every bit as bad as the disease) to causing a FORTIFY abort in Q/API 29. but, yeah, since it doesn't do what the C standard says in any version of Android that the NDK still supports, it makes sense for clang to just reject it regardless of API level.

in case anyone's thinking "citation needed": search for %n on https://android.googlesource.com/platform/bionic/+/master/docs/status.md

In D117611#3256094, @enh wrote:

In D117611#3256089, @phosek wrote:

@srhines can you also take a look at the Android changes or suggest other reviewers?

sorry, took me this long to log in :-(

yeah, lgtm, thanks! %n hasn't worked for about 10 years; it changed from being silently ignored (which, in retrospect, was a cure every bit as bad as the disease) to causing a FORTIFY abort in Q/API 29. but, yeah, since it doesn't do what the C standard says in any version of Android that the NDK still supports, it makes sense for clang to just reject it regardless of API level.

In D117611#3256089, @phosek wrote:

@srhines can you also take a look at the Android changes or suggest other reviewers?

In D111833#3094868, @mbenfield wrote:

Previously this patch did not cover %c and %[, but now it does.

add tests for %c and %[ too? (it's genuinely unclear to me from a quick skim whether this patch covers them.)

"shut up and take my money!" :-)

In D102790#2774272, @pirama wrote:

I'm going to abandon this incarnation of the patch. Once Android has a well-defined behavior of this interaction, we can call at_quick_exit here under a #ifdef.

yeah, https://reviews.llvm.org/D90188 sounds better to me too.

In D90897#2385929, @MaskRay wrote:

In D90897#2385176, @jhenderson wrote:

In D90897#2379198, @MaskRay wrote:

In D90897#2378385, @jhenderson wrote:

Won't this fail if the PT_LOAD the PT_TLS should be in has a zero size too?

PT_LOAD with p_memsz is invalid on Linux. Attempting to mmap a zero-sized segment will fail.

It is both Linux and FreeBSD (according to rL288808). LLD has removeEmptyPTLoad.

This is an implementation detail that won't necessarily be true for all systems. Kernels could also quite happily ignore zero-sized segments with a trivial piece of code to check the size before calling mmap.

sorry about the breakage... thanks for testing on real hardware!

In D86321#2229537, @vvereschaka wrote:

The test got failed again. I got the following output:

ubuntu@jetson8:~$ ./t.tmp.exe
Testing 29715 symbols.
1ILeeeEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE -> <-0x1.cecececececececececececececep+11983L
Got instead: <-0x1.cecececececececececececececep+11983L>
t.tmp.exe: C:/buildbot/temp/llvm-project/libcxxabi/test/test_demangle.pass.cpp:30001: void testFPLiterals(): Assertion `false' failed.
Aborted (core dumped)

looks like it does not expect trailing >.

Address vvereschaka's comment.

In D77924#2229360, @vvereschaka wrote:

I ran the test directly on our aarch64 board.

In D77924#2228309, @RKSimon wrote:

@enh This is causing a test failure on the windows/aarch64 cross compile bot, please can you take a look:

http://lab.llvm.org:8011/builders/llvm-clang-win-x-aarch64/builds/2267
http://lab.llvm.org:8011/builders/llvm-clang-win-x-aarch64/builds/2267/steps/test-libc%2B%2Babi/logs/FAIL%3A%20libc%2B%2Babi%3A%3A%20test_demangle.pass.cpp

(sorry, didn't see these comments until after i'd git pushed. i've failed to set up gmail filters so i see the mail i need to see without drowning in thousands of mails i don't need to see :-( )

clang-format the new test (but leave the other 30kloc).

In D77924#2089802, @ldionne wrote:

In D77924#2088349, @enh wrote:

In D77924#1977737, @ldionne wrote:

Is it possible to add a test to always trigger this bug?

only if you run with hwasan.

Do the current tests fail under hwasan? If not, please add a test case that fails under hwasan, it's better than nothing.

Add test.

Use actual upper bound.

In D77924#1977737, @ldionne wrote:

Is it possible to add a test to always trigger this bug?

actually, it looks like clang is inlining the code for all the is*_l
functions anyway. you're paying for the indirection on
strcoll_l/strxfrm_l/wcscoll_l/wcsxfrm_l, but no one should be using
those anyway.

honestly, if it was me, i'd keep all these inlines even for current releases: inlining a call to the underlying function saves you a layer of useless cruft at runtime. (the Android "implementations" are just one-liners that drop the extra argument and call the underlying function, just like these inlines.)

i assume the idea here is "all libc++ mutexes are covered by RAII anyway, so there's no value to the error checking"?

yeah, we always used the low numbers internally.

not really though --- i think you're getting that from libgcc. so
although short term you don't need to add this stuff to compiler-rt to
test, it will be necessary for anyone not using libgcc. (like us, at
some point in the future.)