Page MenuHomePhabricator

[CMake][Compiler-rt] Make it possible to configure standalone compiler-rt without `LLVMConfig.cmake`.
ClosedPublic

Authored by delcypher on Tue, Mar 30, 2:32 PM.

Details

Summary

Previously it wasn't possible to configure a standalone compiler-rt
build if the LLVMConfig.cmake file isn't present in a shipped
toolchain.

This patch adds a fallback behaviour for when LLVMConfig.cmake is not
available in the toolchain being used for configure. The fallback
behaviour mocks out the bare minimum required to make a configure
succeed when the host is Darwin. Support for other platforms could
be added in future patches.

The new code path is taken either in one of the following cases:

  • llvm-config is not available.
  • llvm-config is available but it provides an invalid path for the CMake files.

The motivation here is to be able to generate the compiler-rt lit test
suites for an arbitrary LLVM toolchain and then run the tests against
it.

The invocation to do this looks something like.

CC=/path/to/cc \
CXX=/path/to/c++ \
cmake \
  -G Ninja \
  -DLLVM_CONFIG_PATH=/path/to/llvm-config \
  -DCOMPILER_RT_INCLUDE_TESTS=ON \
  /path/to/llvm-project/compiler-rt

 # Note we don't compile compiler-rt in this workflow.
bin/llvm-lit -v test/path/to/generated/test_suite

A possible alternative approach is to configure the
cmake/modules/LLVMConfig.cmake.in file in the LLVM source tree
and then include it. This approach was not taken because it is more
complicated.

An interesting side benefit of this patch is that it is now
possible to configure on Darwin without llvm-config being available
by configuring with -DLLVM_CONFIG_PATH="". This moves us a step
closer to a world where no LLVM build artefacts are required to
build compiler-rt.

rdar://76016632

Diff Detail

Event Timeline

delcypher created this revision.Tue, Mar 30, 2:32 PM
delcypher requested review of this revision.Tue, Mar 30, 2:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Mar 30, 2:32 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Do you know what we need LLVMConfig.cmake for? Could we remove that dependency altogether? It's not used by any other runtime as far as I'm aware.

Do you know what we need LLVMConfig.cmake for? Could we remove that dependency altogether? It's not used by any other runtime as far as I'm aware.

I wouldn't want to remove the dependency at this stage as not enough of this new code has been tested (I only tested on macOS). However, I think this patch could used as a stepping stone towards completely removing the dependency on LLVMConfig.cmake. If we landed this and did the necessary work to make this new code path work on all the host platforms we care about then I think we could remove the dependency on LLVMConfig.cmake. I'd like to see that happen because the requirement makes testing harder and seems unnecessary.

The actions that the new compiler_rt_mock_llvm_cmake_config macro take do what I found to be necessary to get the CMake configure to work without having LLVMConfig.cmake present with on a macOS host. Basically this seems to be

  • Setting TARGET_TRIPLE. The build system fails in weird and wonderful ways when this is an empty string. SHAKES FIST AT CMAKE.
  • Including AddLLVM.cmake from the LLVM source tree. It seems we have some CMake code that depends on code in this file.
  • Add LLVM's CMake module directory into the CMake module search path. This necessary for the include of AddLLVM.cmake to work.

It may turn out we need to do more than this to support other platforms.

Do you know what we need LLVMConfig.cmake for? Could we remove that dependency altogether? It's not used by any other runtime as far as I'm aware.

I wouldn't want to remove the dependency at this stage as not enough of this new code has been tested (I only tested on macOS). However, I think this patch could used as a stepping stone towards completely removing the dependency on LLVMConfig.cmake. If we landed this and did the necessary work to make this new code path work on all the host platforms we care about then I think we could remove the dependency on LLVMConfig.cmake. I'd like to see that happen because the requirement makes testing harder and seems unnecessary.

SGTM

The actions that the new compiler_rt_mock_llvm_cmake_config macro take do what I found to be necessary to get the CMake configure to work without having LLVMConfig.cmake present with on a macOS host. Basically this seems to be

  • Setting TARGET_TRIPLE. The build system fails in weird and wonderful ways when this is an empty string. SHAKES FIST AT CMAKE.

I see only two dependencies on this variable in https://github.com/llvm/llvm-project/blob/a360a9786f5f82f4beff6fdcec12b40ee392db7a/compiler-rt/cmake/Modules/CompilerRTUtils.cmake#L317 and https://github.com/llvm/llvm-project/blob/a360a9786f5f82f4beff6fdcec12b40ee392db7a/compiler-rt/cmake/Modules/AddCompilerRT.cmake#L131.

I could see how the build starts misbehaving if that variable isn't set, but it should be also feasible to address that issue given that it's only two references.

  • Including AddLLVM.cmake from the LLVM source tree. It seems we have some CMake code that depends on code in this file.
  • Add LLVM's CMake module directory into the CMake module search path. This necessary for the include of AddLLVM.cmake to work.

This should become a non-issue if we assume the monorepo layout.

It may turn out we need to do more than this to support other platforms.

compiler-rt/cmake/Modules/CompilerRTUtils.cmake
380

Would it be possible to leave a comment to clarify that this configuration is likely insufficient for a full build and is only here to allow testing?

Do you know what we need LLVMConfig.cmake for? Could we remove that dependency altogether? It's not used by any other runtime as far as I'm aware.

I wouldn't want to remove the dependency at this stage as not enough of this new code has been tested (I only tested on macOS). However, I think this patch could used as a stepping stone towards completely removing the dependency on LLVMConfig.cmake. If we landed this and did the necessary work to make this new code path work on all the host platforms we care about then I think we could remove the dependency on LLVMConfig.cmake. I'd like to see that happen because the requirement makes testing harder and seems unnecessary.

SGTM

The actions that the new compiler_rt_mock_llvm_cmake_config macro take do what I found to be necessary to get the CMake configure to work without having LLVMConfig.cmake present with on a macOS host. Basically this seems to be

  • Setting TARGET_TRIPLE. The build system fails in weird and wonderful ways when this is an empty string. SHAKES FIST AT CMAKE.

I see only two dependencies on this variable in https://github.com/llvm/llvm-project/blob/a360a9786f5f82f4beff6fdcec12b40ee392db7a/compiler-rt/cmake/Modules/CompilerRTUtils.cmake#L317 and https://github.com/llvm/llvm-project/blob/a360a9786f5f82f4beff6fdcec12b40ee392db7a/compiler-rt/cmake/Modules/AddCompilerRT.cmake#L131.

Those are the dependencies I ran into too but some of the test files depend on it too.

https://github.com/llvm/llvm-project/blob/a360a9786f5f82f4beff6fdcec12b40ee392db7a/compiler-rt/test/fuzzer/lit.site.cfg.py.in#L14
https://github.com/llvm/llvm-project/blob/a360a9786f5f82f4beff6fdcec12b40ee392db7a/compiler-rt/unittests/lit.common.unit.configured.in#L4

I could see how the build starts misbehaving if that variable isn't set, but it should be also feasible to address that issue given that it's only two references.

Yep, that's what compiler_rt_mock_llvm_cmake_config_set_target_triple effectively does. I'm wondering if it should just use ${CMAKE_C_COMPILER_TARGET} rather than calling llvm-config?

  • Including AddLLVM.cmake from the LLVM source tree. It seems we have some CMake code that depends on code in this file.
  • Add LLVM's CMake module directory into the CMake module search path. This necessary for the include of AddLLVM.cmake to work.

This should become a non-issue if we assume the monorepo layout.

Yep. That's why this patch depends on https://reviews.llvm.org/D99620. The compiler_rt_mock_llvm_cmake_config_include_cmake_files() and compiler_rt_mock_llvm_cmake_config_set_cmake_path() macros do the necessary include via a path that assumes the monorepo.

It may turn out we need to do more than this to support other platforms.

I think ideally we wouldn't have CompilerRTMockLLVMCMakeConfig.cmake at all and instead just fixed all the issues at the right places in the CMake code. Right now though it seems like a good self contained place to mimic the things from LLVMConfig.cmake that we depend on. If this file is updated to allow configure on all platforms to succeed then at all point in time I think we could remove CompilerRTMockLLVMCMakeConfig.cmake and make the necessary fixes. Unfortunately, I don't have the bandwidth right now to do a bunch of testing on other platforms.

delcypher updated this revision to Diff 334298.Tue, Mar 30, 4:10 PM
  • Add comment clarifying the intended use case of the configuration that doesn't depend on LLVMConfig.cmake.
delcypher marked an inline comment as done.Tue, Mar 30, 4:11 PM
delcypher added inline comments.
compiler-rt/cmake/Modules/CompilerRTUtils.cmake
380

Sure. Done.

On Linux "CC=gcc CXX=g++ cmake -G Ninja -DCOMPILER_RT_INCLUDE_TESTS=ON ../../llvm-project/compiler-rt" works for me without the patch

delcypher marked an inline comment as done.Tue, Mar 30, 4:31 PM

On Linux "CC=gcc CXX=g++ cmake -G Ninja -DCOMPILER_RT_INCLUDE_TESTS=ON ../../llvm-project/compiler-rt" works for me without the patch

@vitalybuka I suspect CMake is finding a copy of llvm-config to use and that the path printed out by llvm-config --cmake-dir points into a directory that has usable CMake files generated by an LLVM build. This patch is trying to address the case when those CMake files from an LLVM build are not available (i.e. the toolchain doesn't ship them).

If you invoke CMake with -DLLVM_CONFIG_PATH=/path/to/llvm-config where /path/to/llvm-config points to a directory that doesn't exist then you should run into the problem that this patch is trying to resolve. You can likely simulate that by temporarily renaming the lib/cmake directory in the root of an LLVM build directory and using the llvm-config binary in bin/ in the root of an LLVM build tree for the standalone compiler-rt configure.

On Linux "CC=gcc CXX=g++ cmake -G Ninja -DCOMPILER_RT_INCLUDE_TESTS=ON ../../llvm-project/compiler-rt" works for me without the patch

@vitalybuka I suspect CMake is finding a copy of llvm-config to use and that the path printed out by llvm-config --cmake-dir points into a directory that has usable CMake files generated by an LLVM build.

It's fully possible to build compiler-rt without that, too, see lines 216-220 in CompilerRTUtils.cmake, with the "UNSUPPORTED COMPILER-RT CONFIGURATION DETECTED: llvm-config not found. Reconfigure with -DLLVM_CONFIG_PATH=path/to/llvm-config." warning. It works fine to build it despite that warning, dunno what I'm missing. I guess I can't build/run tests in that config though.

delcypher updated this revision to Diff 335006.Fri, Apr 2, 12:04 PM

@mstorsjo @phosek I've adapted this patch to changes in https://reviews.llvm.org/D99620 and I've hoisted the code out of if (LLVM_CONFIG_PATH). This required me to rewrite compiler_rt_mock_llvm_cmake_config_set_target_triple so that it no longer uses llvm-config because we can't assume it's available anymore. Instead we inspect the compiler being used and try to get a target triple from it.

A nice side benefit of the hoisting is that on Darwin I can actually now configure without needing llvm-config or LLVMConfig.cmake to be available.

delcypher edited the summary of this revision. (Show Details)Fri, Apr 2, 12:54 PM
delcypher edited the summary of this revision. (Show Details)
vitalybuka resigned from this revision.Fri, Apr 2, 1:00 PM
mstorsjo added inline comments.Fri, Apr 2, 1:55 PM
compiler-rt/cmake/Modules/CompilerRTMockLLVMCMakeConfig.cmake
37

Clang supports -dumpmachine too - is there any functional difference between the two?

48

If building with e.g. -DCMAKE_C_COMPILER_TARGET=<triple> -DCOMPILER_RT_DEFAULT_TARGET_ONLY=TRUE, which sets TARGET_TRIPLE somehow (I don't know offhand where/how), I guess that we shouldn't try to do this detection here, but just go with whatever was provided that way?

It also seems like it's possible to build by just setting TARGET_TRIPLE only (without setting COMPILER_RT_DEFAULT_TARGET_ONLY) - that kinda works but has odd/unexpected effects for me (by building both i386 and x86_64 at the same time, when I've been used to only building one arch at a time).

delcypher added inline comments.Fri, Apr 2, 5:27 PM
compiler-rt/cmake/Modules/CompilerRTMockLLVMCMakeConfig.cmake
37

Not that I know of. I wrote support for clang first and then I thought I should probably add gcc support just incase that's what you were using to build compiler-rt. I can merge the branches to use the same flag if that's what you prefer.

48

If building with e.g. -DCMAKE_C_COMPILER_TARGET=<triple> -DCOMPILER_RT_DEFAULT_TARGET_ONLY=TRUE, which sets TARGET_TRIPLE somehow (I don't know offhand where/how),

Are you sure about this? I can't reproduce when running like this (without this patch applied)

cmake  -GNinja -DLLVM_CONFIG_PATH="" -DCMAKE_C_COMPILER_TARGET=x86_64-apple-darwin20.3.0  -DCOMPILER_RT_DEFAULT_TARGET_ONLY=TRUE -DCOMPILER_RT_INCLUDE_TESTS=ON /path/compiler-rt

Where are you looking for the value of TARGET_TRIPLE to show up? One place you can look for it in your build directory is unittests/lit.common.unit.configured. I see

config.target_triple = ""

Note don't look at test/lit.common.configured. That has COMPILER_RT_DEFAULT_TARGET_TRIPLE expanded and not TARGET_TRIPLE into the "target_triple" lit config, which is extremely confusing.

I guess that we shouldn't try to do this detection here, but just go with whatever was provided that way?

We could... I guess we're interacting with this existing code...

macro(construct_compiler_rt_default_triple)
  if(COMPILER_RT_DEFAULT_TARGET_ONLY)
    if(DEFINED COMPILER_RT_DEFAULT_TARGET_TRIPLE)
      message(FATAL_ERROR "COMPILER_RT_DEFAULT_TARGET_TRIPLE isn't supported when building for default target only")
    endif()
    set(COMPILER_RT_DEFAULT_TARGET_TRIPLE ${CMAKE_C_COMPILER_TARGET})
  else()
    set(COMPILER_RT_DEFAULT_TARGET_TRIPLE ${TARGET_TRIPLE} CACHE STRING
          "Default triple for which compiler-rt runtimes will be built.")
  endif()
  ...

So I guess compiler_rt_mock_llvm_cmake_config_set_target_triple could have an early exit... to use CMAKE_C_COMPILER_TARGET if COMPILER_RT_DEFAULT_TARGET_ONLY is set. However, there's no point doing that if TARGET_TRIPLE is actually getting set elsewhere because the results will just overwritten.

Can you confirm that TARGET_TRIPLE is actually being set using the method I outlined above?

It also seems like it's possible to build by just setting TARGET_TRIPLE only (without setting COMPILER_RT_DEFAULT_TARGET_ONLY) - that kinda works but has odd/unexpected effects for me (by building both i386 and x86_64 at the same time, when I've been used to only building one arch at a time).

I'd said this is both expected and unexpected. I'd expect that because compiler-rt normally builds for multiple architectures (at least it does in the configurations I normally use) and so the name of COMPILER_RT_DEFAULT_TARGET_ONLY does what it suggests, it builds only one target. So if you don't use the option you get builds for multiple architectures.

It's also a little unexpected because having TARGET_TRIPLE be a single architecture might suggest that's the only thing compiler-rt should compile for. But as you've seen that's not what happens.

The fact that this variable exists at all and is used is feels like a build system design mistake but fixing that isn't really in the scope of this patch.

mstorsjo added inline comments.Sat, Apr 3, 12:51 AM
compiler-rt/cmake/Modules/CompilerRTMockLLVMCMakeConfig.cmake
37

I guess that'd be a bit more straightforward to handle both gcc and clang with the same, maybe with a comment that clang also supports -print-target-triple.

48

Sorry - I wasn't sure it was setting TARGET_TRIPLE - I just kinda assumed that was the internal variable that ends up set.

If I configure that way, I actually get the default TARGET_TRIPLE of the llvm build (provided that llvm-config can be found), not the one I set in CMAKE_C_COMPILER_TARGET.

So, sorry for the confusion.

(But I see that when building via llvm/runtimes, it sets both COMPILER_RT_DEFAULT_TARGET_ONLY, and I think, TARGET_TRIPLE, but not CMAKE_C_COMPILER_TARGET, but when I try doing that myself, it errors out due to the list of architectures ending up empty, " -- Compiler-RT supported architectures:")

Doing anything about the case of building multiple architectures at once is out of scope indeed. (Do you happen to know how to request what set of architectures to build for, can I request building for eg i386, x86_64, armv7 and aarch64 at once, of is it just all x86 variants at once?)

delcypher updated this revision to Diff 335112.Sat, Apr 3, 2:44 PM
  • Handle gcc and clang in the same branch.
  • Prefer CMAKE_C_COMPILER_TARGET to compute TARGET_TRIPLE if it and COMPILER_RT_DEFAULT_TARGET_ONLY are specified.
delcypher marked 2 inline comments as done.Sat, Apr 3, 2:51 PM

Thanks, this seems to work fine in my setups now!

When building via llvm/runtimes (which I don't use so I don't have a test setup handy), I believe that sets TARGET_TRIPLE somehow. Does that end up trying to probe the triple from the compiler in that case? I guess it'd always be correct, but if we'd avoid the whole probing in that case too, that'd be great. Maybe it already does that - or the runtimes build does thing differently somehow? (I've just tried to read llvm/runtimes/CMakeLists.txt.) Or maybe we end up in a case where these routines aren't called at all in such a build?

I might be able to try to set up such a build configuration in a couple days to try that out though...

delcypher added inline comments.Sat, Apr 3, 3:19 PM
compiler-rt/cmake/Modules/CompilerRTMockLLVMCMakeConfig.cmake
48

Do you happen to know how to request what set of architectures to build for, can I request building for eg i386, x86_64, armv7 and aarch64 at once, of is it just all x86 variants at once?)

I don't know of a way. Looking at test_targets() in cmake/config-ix.cmake that ends up populating the COMPILER_RT_SUPPORTED_ARCH list with of architectures that test_targets() managed to detect support for (provided the host isn't an Apple platform). Later on in cmake/config-ix.cmake there are calls to filter_available_targets() (not on Apple platforms) that set the supported targets for various different sanitizers. The variables here look like normal variables rather than CMake cache variables so I don't think you'll be able to override the behaviour here using -DCOMPILER_RT_SUPPORTED_ARCH unless some changes are made to the existing code. I could be wrong about this though...

Thanks, this seems to work fine in my setups now!

When building via llvm/runtimes (which I don't use so I don't have a test setup handy), I believe that sets TARGET_TRIPLE somehow. Does that end up trying to probe the triple from the compiler in that case? I guess it'd always be correct, but if we'd avoid the whole probing in that case too, that'd be great. Maybe it already does that - or the runtimes build does thing differently somehow? (I've just tried to read llvm/runtimes/CMakeLists.txt.) Or maybe we end up in a case where these routines aren't called at all in such a build?

I might be able to try to set up such a build configuration in a couple days to try that out though...

I'm not sure. Reading that code is a little confusing...

So there are external project invocations that pass

  1. LLVM_DEFAULT_TARGET_TRIPLE. That looks like it's only used in compiler-rt to help with finding GNU gold/ld. So that probably doesn't matter here.
  2. TARGET_TRIPLE as an argument to the llvm_ExternalProject_Add() function

The second one is a bit confusing.

There's this code inside llvm_ExternalProject_Add

if(NOT ARG_TARGET_TRIPLE)
  set(target_triple ${LLVM_DEFAULT_TARGET_TRIPLE})
else()
  set(target_triple ${ARG_TARGET_TRIPLE})
endif()

is_msvc_triple(is_msvc_target ${target_triple})

but then target_triple doesn't get used again... which makes no sense. (@phosek @beanz is this a bug?).

Then later on I do see

if(ARG_TARGET_TRIPLE)
  list(APPEND compiler_args -DCMAKE_C_COMPILER_TARGET=${ARG_TARGET_TRIPLE})
  list(APPEND compiler_args -DCMAKE_CXX_COMPILER_TARGET=${ARG_TARGET_TRIPLE})
  list(APPEND compiler_args -DCMAKE_ASM_COMPILER_TARGET=${ARG_TARGET_TRIPLE})
endif()

So that does suggest Compiler-RT will have CMAKE_C_COMPILER_TARGET passed. The builtins and other runtime builds in llvm-project/llvm/runtimes look like they pass -DCOMPILER_RT_DEFAULT_TARGET_ONLY=ON.

So that would suggest that both CMAKE_C_COMPILER_TARGET and COMPILER_RT_DEFAULT_TARGET_ONLY get set.

If I've read this correctly that means this patch doesn't need to change because it already checks for CMAKE_C_COMPILER_TARGET and COMPILER_RT_DEFAULT_TARGET_ONLY being set and defers to using CMAKE_C_COMPILER_TARGET to set TARGET_TRIPLE in that case.

I don't use the llvm-project/llvm/runtimes directory either so I don't know if it works on Apple platforms. I'd be a bit happier if someone who does use it checked that this patch didn't break anything.

So that would suggest that both CMAKE_C_COMPILER_TARGET and COMPILER_RT_DEFAULT_TARGET_ONLY get set.

If I've read this correctly that means this patch doesn't need to change because it already checks for CMAKE_C_COMPILER_TARGET and COMPILER_RT_DEFAULT_TARGET_ONLY being set and defers to using CMAKE_C_COMPILER_TARGET to set TARGET_TRIPLE in that case.

Thanks for digging into that! That sounds plausible, and that narrows down the interfaces towards how compiler-rt actually gets configured a little bit.

Thanks, this seems to work fine in my setups now!

When building via llvm/runtimes (which I don't use so I don't have a test setup handy), I believe that sets TARGET_TRIPLE somehow. Does that end up trying to probe the triple from the compiler in that case? I guess it'd always be correct, but if we'd avoid the whole probing in that case too, that'd be great. Maybe it already does that - or the runtimes build does thing differently somehow? (I've just tried to read llvm/runtimes/CMakeLists.txt.) Or maybe we end up in a case where these routines aren't called at all in such a build?

I might be able to try to set up such a build configuration in a couple days to try that out though...

I'm not sure. Reading that code is a little confusing...

So there are external project invocations that pass

  1. LLVM_DEFAULT_TARGET_TRIPLE. That looks like it's only used in compiler-rt to help with finding GNU gold/ld. So that probably doesn't matter here.
  2. TARGET_TRIPLE as an argument to the llvm_ExternalProject_Add() function

The second one is a bit confusing.

There's this code inside llvm_ExternalProject_Add

if(NOT ARG_TARGET_TRIPLE)
  set(target_triple ${LLVM_DEFAULT_TARGET_TRIPLE})
else()
  set(target_triple ${ARG_TARGET_TRIPLE})
endif()

is_msvc_triple(is_msvc_target ${target_triple})

but then target_triple doesn't get used again... which makes no sense. (@phosek @beanz is this a bug?).

This is a recent addition, but if I read the correctly, is_msvc_triple sets is_msvc_target based on the value of target_triple. I think the fact that the output variable is first is confusing.

Then later on I do see

if(ARG_TARGET_TRIPLE)
  list(APPEND compiler_args -DCMAKE_C_COMPILER_TARGET=${ARG_TARGET_TRIPLE})
  list(APPEND compiler_args -DCMAKE_CXX_COMPILER_TARGET=${ARG_TARGET_TRIPLE})
  list(APPEND compiler_args -DCMAKE_ASM_COMPILER_TARGET=${ARG_TARGET_TRIPLE})
endif()

So that does suggest Compiler-RT will have CMAKE_C_COMPILER_TARGET passed. The builtins and other runtime builds in llvm-project/llvm/runtimes look like they pass -DCOMPILER_RT_DEFAULT_TARGET_ONLY=ON.

So that would suggest that both CMAKE_C_COMPILER_TARGET and COMPILER_RT_DEFAULT_TARGET_ONLY get set.

If I've read this correctly that means this patch doesn't need to change because it already checks for CMAKE_C_COMPILER_TARGET and COMPILER_RT_DEFAULT_TARGET_ONLY being set and defers to using CMAKE_C_COMPILER_TARGET to set TARGET_TRIPLE in that case.

I don't use the llvm-project/llvm/runtimes directory either so I don't know if it works on Apple platforms. I'd be a bit happier if someone who does use it checked that this patch didn't break anything.

Your understanding is correct. The only case where this isn't true and we rely on automatic detection even in the llvm-project/llvm/runtimes is on Apple platforms because the compiler-rt CMake setup works differently there due to universal binary support.

phosek accepted this revision.Tue, Apr 6, 12:34 AM

LGTM (I've tested this together with D99621 locally for the Fuchsia build and everything seems to be working).

This revision is now accepted and ready to land.Tue, Apr 6, 12:34 AM

LGTM (I've tested this together with D99621 locally for the Fuchsia build and everything seems to be working).

Great. Thanks for testing and thanks for the review.