This is an archive of the discontinued LLVM Phabricator instance.

[scudo] Tune FuchsiaConfig for RiscV
ClosedPublic

Authored by Caslyn on Apr 16 2023, 12:42 PM.

Details

Summary

Reduce PrimaryRegionSizeLog to 28U to be compatible with a 39 bit
VMA on the fuchsia-riscv platform. PrimaryGroupSizeLog is
reduced to 19 to preserve 512 BatchGroups per region.

This change can be tested on Fuchsia with:

fx set --auto-dir bringup.riscv64 --with //bundles:boot_tests \
fx build bundles:boot_tests \
fx run-boot-test --args={-s,1} boot-libc-unittests \
--cmdline='--gtest_filter=-*DeathTest*:PthreadGetSet*:ScudoSecondaryTest*'

The gtest filter ignores pthread and death tests due to non-scudo
related issues on fuchsia-riscv (ScudoSecondaryTest includes a death
check).

Related Ticket: https://fxbug.dev/125263

Diff Detail

Event Timeline

Caslyn created this revision.Apr 16 2023, 12:42 PM
Caslyn requested review of this revision.Apr 16 2023, 12:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2023, 12:42 PM
Herald added subscribers: Restricted Project, pcwang-thead, eopXD. · View Herald Transcript
Caslyn updated this revision to Diff 514046.Apr 16 2023, 1:01 PM

Fix typo

Caslyn edited the summary of this revision. (Show Details)Apr 16 2023, 1:03 PM
fabio-d requested changes to this revision.Apr 17 2023, 8:52 AM
fabio-d added inline comments.
compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
112 ↗(On Diff #514046)

I would prefer to keep it for now even if it's deprecated (as @Chia-hungDuan mentioned in https://reviews.llvm.org/D146454#4262007). The reason why I'm asking it is that this configuration actually helped detect an issue in the recent MemMap CLs because, unlike FuchsiaConfig, it enables the secondary allocator cache. We can remove it later when we ensure that other tests that would have caught those issues exist.

This revision now requires changes to proceed.Apr 17 2023, 8:52 AM
Chia-hungDuan added inline comments.Apr 17 2023, 9:18 AM
compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
112 ↗(On Diff #514046)

Same here. If you decide to remove this it'll be better to do it separately. OTOH, you may consider using (or adding) other config that enables secondary cache instead of using AndroidSvelte if you still want to verify the secondary cache.

Caslyn updated this revision to Diff 514368.Apr 17 2023, 12:43 PM
Caslyn marked 2 inline comments as done.

Add back AndroidSvelteConfig to Fuchsia combined tests.

compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
112 ↗(On Diff #514046)

Thanks for flagging, I've added this back. It was a mistake for me to remove it for this CL. I misinterpreted the failure of ScudoCombinedDeathTestBasicCombined14_AndroidSvelteConfig on fuchsia-riscv to be due to the config, but it failed because of a separate issue with death-tests on fuchsia-riscv.

Caslyn edited the summary of this revision. (Show Details)Apr 17 2023, 12:52 PM
fabio-d accepted this revision.Apr 17 2023, 12:56 PM
This revision is now accepted and ready to land.Apr 17 2023, 12:56 PM
This revision was landed with ongoing or failed builds.Apr 17 2023, 1:16 PM
This revision was automatically updated to reflect the committed changes.