In D137268#4069935, @enh wrote:

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).

bionic's tests check for va_list, because that's what POSIX says will be visible. ISO C doesn't say that, so i think the _intention_ for glibc -- musl seems to do this correctly -- is to say "if we're only compiling C source, you don't get va_list, but if we're compiling POSIX source, you do get va_list". so i think this __gnuc_va_list thing is their workaround to still export the _functions_ without exporting the _type_ for ISO C?

In D137268#4069779, @enh wrote:

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?

https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/basedefs/wchar.h.html says:
"""
The <wchar.h> header shall define the following types:
...
va_list
[CX] As described in <stdarg.h>.
"""
which is why i think our "<wchar.h> exports va_list" test is correct. (Android doesn't support an "ISO only" mode --- you're effectively always in _POSIX_SOURCE mode, so we build the test against glibc with _POSIX_SOURCE defined.)

Another project that is also breaks with this change is glibc. Although I could be fixed on glibc side, the undefined symbols are used to simplify the version symbol for multiple ABI with different base ABIs.

Update review based on reviewers comments.

In D135565#3885326, @wzssyqa wrote:

On a Gentoo mips64 machine, libgcc.a:_clear_cache.o defines __clear_cache as a no-op:

0000000000000000 <__clear_cache>:
   0:   03e00008        jr      ra
   4:   00000000        nop
        ...

This should be a gcc's bug.
Luckily, we can use rdhwr+synci for llvm, since we only support r2+.
I will try to fix gcc, too.

In D135565#3884566, @MaskRay wrote:

OK, I see that syscall is not defined in -std=c* modes (in glibc and musl) without _DEFAULT_SOURCE/_GNU_SOURCE. Is cacheflush a better API than _flush_cache? In musl it's always available, but in glibc, it requires _DEFAULT_SOURCE like syscall:

In D131162#3708701, @MaskRay wrote:

The SHF_GNU_RETAIN change is more so that GNU ld recognizes its semantics, less about the readelf -s output. With many systems adopting GNU ABI features, I do not know whether it makes significant sense to "upgrade" ELFOSABI_NONE to ELFOSABI_GNU.

It does help on test-suite, thanks.

This commit seems to have broken test-suite on aarch64 [1]. The warning shows:

LGTM, thanks.

In D129471#3643096, @MaskRay wrote:

In D129471#3642148, @zatrazz wrote:

LGTM, the kernel exported value is same for all architectures. Anther possibility would be to split the sanitizer_platform_limits_posix.cpp and add a sanitizer_platform_limits_posix_linux.cpp which only add linux includes.

If we are going to do this, sanitizer_platform_limits_linux.cpp may be the choice.

Do you think other #include <linux/*.h> in this file may cause a potential conflict in the future?

LGTM, the kernel exported value is same for all architectures. Anther possibility would be to split the sanitizer_platform_limits_posix.cpp and add a sanitizer_platform_limits_posix_linux.cpp which only add linux includes.

I tested this patch on my glibc clang branch and while it has fixed the st_value aliases issues, it now triggers another regression:

Would avoid calling sched_yield() help in this situation? Depending of the system default, calling the glibc symbol might result in a lazy-resolution and thus invoke a lot of loader's code (which might clobber temporary registers).

This has caused a regression on stage2 buildbot [1], more specifically the test FAIL: Clang::p4-0x.cpp (but I think the other others might be related).

Hi, I am not 100% sure but it seems to trigger a regression on aarch64 bot [1]. I can reproduce with current master as well:

The change look reasonable to me, I will increase build time but it increase test coverage.

LGTM, thanks.

Updated patch based on previous comments.

In D96803#2569436, @aeubanks wrote:

In D96803#2568179, @zatrazz wrote:

In D96803#2566322, @aeubanks wrote:

why is this now a module pass?

Mainly to avoid the default rule from new pass manager to *not* apply any FunctionPass for optnone (which is the main issue for PR49143). Is there a better way to accomplish it? I noted also that createModuleToFunctionPassAdaptor basically creates a adaptor that applies the pass to all function on the module.

It's always good to make the pass as specific as possible (e.g. prefer a function pass rather than a module pass) so it doesn't have to worry about infra. For example, just iterating over functions doesn't skip declarations.

In D96803#2566322, @aeubanks wrote:

why is this now a module pass?

LGTM, it fixes the arm sanitizer regressions.

This change broke arm32 llvm-symbolizer which triggered some sanitizer regression. Previously, ARM mapping symbols [1] were ignored but after this change they are shown as the function name.

In D96490#2556620, @ostannard wrote:

The test-suite is the only thing this bot does to test global-isel, so I don't think there's any point in running the bot with this change. I think our options here are:

• Disable just the tests which are failing (looks like it's just test-suite::GCC-C-execute-eeprof-1.test?)
• Convert this to a 2-stage bot with the second stage built using global-isel, so that we are still doing some useful testing
• Disable this bot completely until https://bugs.llvm.org/show_bug.cgi?id=49143 is fixed.

Updated patch based on previous comments.

In D96337#2556270, @ostannard wrote:

Could this also affect regular users of the sanitizers, or is it specific to the way we are running the tests? If it could affect users, then could we fix this in the clang driver, so that unwind tables are turned on by default whenever the sanitizers need them?

Updated patch based on previous comments. The R_AARCH64_GOT_PAGE is added on
the isStaticLinkTimeConstant and the test is changed to check for shared build as well.

Ping.

Updated patch based on previous comments.

Patch updated from comments, main changes are:

I will update the patch, since it is incomplete and requires https://reviews.llvm.org/D94809 .

Updated patch based on previous version.

Both https://reviews.llvm.org/D93044 and https://reviews.llvm.org/D93235 have been approved, so the prerequisites of this patch are ok to land.

In D93235#2454115, @grimar wrote:

LGTM with 2 minor nits. Thanks!

Updated patch based on previous comment:

Updated patch based on previous comments:

Updated patch based on previous comments. It depends on https://reviews.llvm.org/D93235 for YAML STO_AARCH64_VARIANT_PCS support.

Updated patch based on previous comment.

Updated patch based on previous comments. For in.relaIplt / R_AARCH64_IRELATIVE
I decided to not include since it seems to what binutils is done currently. I will raise with
them if this is the intended behaviour or if R_AARCH64_IRELATIVE should also create
the dynamic tag (and fix is accordingly on lld if it were the case).

Updated patch based on previous comments.

Updated patch based on previous comments.