Build nplb hermetically #4587

niranjanyardi · 2024-12-14T04:17:01Z

b/371241293

base/BUILD.gn

base/allocator/partition_allocator/BUILD.gn

buildtools/third_party/libc++/BUILD.gn

build/config/c++/BUILD.gn

buildtools/third_party/libc++/BUILD.gn

base/BUILD.gn

build/config/c++/BUILD.gn

buildtools/third_party/libc++abi/BUILD.gn

niranjanyardi · 2025-01-14T00:49:31Z

build/config/c++/BUILD.gn

@@ -68,6 +72,18 @@ config("runtime_library") {
      "-isystem" + rebase_path("$libcxx_prefix/include", root_build_dir),
      "-isystem" + rebase_path("$libcxxabi_prefix/include", root_build_dir),
    ]
+    if (is_cobalt && is_cobalt_hermetic_build) {


I get this error:

In file included from ../../buildtools/third_party/libc++/trunk/include/__type_traits/is_trivially_copyable.h:14:
../../buildtools/third_party/libc++/trunk/include/cstdint:149:5: error: tried including <stdint.h> but didn't find libc++'s <stdint.h> header. This usually means that your header search paths are not configured properly. The header search paths should contain the C++ Standard Library headers before any C Standard Library, and you are probably using compiler flags that make that not be the case.

when i just use cflags instead of (cflags_cc and cflags_c)

niranjanyardi · 2025-01-14T01:11:50Z

starboard/configuration.h

-#error "You must define STARBOARD in Starboard builds."
+#include "build/build_config.h"
+
+#if !BUILDFLAG(IS_COBALT)


maybe this whole check needs to be removed ?

had to put IS_COBALT as we need some flag which is on for android and linux and we want to remove STARBOARD, OS_STARBOARD instances and replace them with other flags we ahve defined like IS_COBALT, IS_STARBOARD

andrewsavage1 · 2025-01-14T14:56:30Z

base/BUILD.gn

 # Determines whether libevent should be dep.
-dep_libevent = !is_fuchsia && !is_win && !is_mac && !is_nacl
+if (is_cobalt && is_cobalt_hermetic_build) {
+  dep_libevent = false


If this won't be fixed in the scope of this PR, this variable should ideally be set by linux-evergreen's args.gn file

+1 to add it to the args.gn

andrewsavage1

It looks like this isn't changing any DEPS repos to subtrees—is it no longer needed?

andrewsavage1 · 2025-01-14T15:01:46Z

base/BUILD.gn

+    # TODO: b/384652502 - Cobalt: Fix compiler errors building hermetically.
+    #   ../../base/process/launch_posix.cc:145:18: error: use of undeclared identifier 'SYS_rt_sigaction'
+    # return syscall(SYS_rt_sigaction, sig, act, oact, sizeof(kernel_sigset_t));
+    if (is_cobalt && is_cobalt_hermetic_build) {


Conditions should be written to minimize the number of conditional blocks.

ref: https://gn.googlesource.com/gn/+/main/docs/style_guide.md#conditions

andrewsavage1 · 2025-01-14T15:02:53Z

base/BUILD.gn

@@ -1013,6 +1024,26 @@ component("base") {
    ]
  }

+  # TODO: b/384652502 - Cobalt: Add source files after fixing compiler errors.


Would we add linux-specific files to our evergreen build? At some point in the evergreen build, we may want to make is_linux=false

andrewsavage1 · 2025-01-14T15:03:29Z

base/allocator/partition_allocator/BUILD.gn

@@ -320,6 +329,7 @@ component("partition_alloc") {
    # tagging.cc requires __arm_mte_set_* functions.
    deps += [ "//third_party/android_ndk:cpu_features" ]
  }
+


nit: remove

andrewsavage1 · 2025-01-14T15:05:36Z

base/allocator/partition_allocator/shim/allocator_shim_override_libc_symbols.h

+// /arch/generic --sysroot=../../build/linux/debian_bullseye_amd64-sysroot -fvisibility-inlines-hidden -c ../../base/allocator/partition_allocator/shim/allocator_shim.cc -o obj/base/base/allocator_shim.o
+// In file included from ../../base/allocator/partition_allocator/shim/allocator_shim.cc:409:
+// ../../base/allocator/partition_allocator/shim/allocator_shim_override_libc_symbols.h:36:26: error: exception specification in declaration does not match previous declaration
+#if BUILDFLAG(IS_STARBOARD)


This seems like an incorrect macro check, and I assume it's cruft as you've changed where it's included in base/allocator/partition_allocator/shim/allocator_shim.cc

andrewsavage1 · 2025-01-14T15:06:35Z

base/debug/stack_trace_posix.cc

  // See: https://man7.org/linux/man-pages/man2/exit_group.2.html
  syscall(SYS_exit_group, EXIT_FAILURE);
+#else
+// No clue how to substitue , ask yavor


Seems like something to figure out in the scope of this PR?

This sections shouldn't be run at all, you should change the conditional above to not define AlarmSignalHandler (and not have references to it in other places). Where we call _exit we may want to call SbSystemBreakIntoDebugger instead, though seems to me like we should be supporting _exit in Starboard

The call seem to be needed only as a workaround for AMD Linux shutdowns of the GPU process. We can probably just call SbSystemBreakIntoDebugger or abort.

andrewsavage1 · 2025-01-14T15:15:41Z

starboard/BUILD.gn

@@ -135,7 +132,8 @@ if (!is_cobalt) {
      if (sb_is_evergreen) {
        public_deps += [ "//starboard/elf_loader:sabi_string" ]
      } else {
-        data_deps = [ ":starboard($starboard_toolchain)" ]
+        # Symbols resolved at buildtime, talk to yavor, andrew
+        deps += [ ":starboard($starboard_toolchain)" ]


I believe this is okay

andrewsavage1 · 2025-01-14T15:17:23Z

starboard/common/condition_variable.h

@@ -26,6 +26,10 @@
 #include "starboard/configuration.h"
 #include "starboard/types.h"

+#ifdef __cplusplus


Why is this being added?

andrewsavage1 · 2025-01-14T15:20:09Z

third_party/abseil-cpp/absl/base/internal/raw_logging.cc

@@ -27,6 +27,11 @@
 #include "absl/base/internal/errno_saver.h"
 #include "absl/base/log_severity.h"

+#include "build/build_config.h"
+
+#if BUILDFLAG(IS_STARBOARD) && defined(_LIBCPP_HAS_MUSL_LIBC)


When would IS_STARBOARD be true and _LIBCPP_HAS_MUSL_LIBC be false?

I should add a TODO above this . Currently _LIBCPP_HAS_MUSL_LIBC is only for the linux hermetic build

We wouldn't have non-hermetic IS_STARBOARD. Is this a transitional step? Also _LIBCPP_HAS_MUSL_LIBC is a variable only for libcxx. It doesn't make sense to use it in any other modules.

andrewsavage1 · 2025-01-14T15:22:11Z

third_party/abseil-cpp/absl/base/internal/raw_logging.cc

@@ -211,6 +217,8 @@ void AsyncSignalSafeWriteToStderr(const char* s, size_t len) {
  write(STDERR_FILENO, s, len);
 #elif defined(ABSL_HAVE_RAW_IO)
  _write(/* stderr */ 2, s, static_cast<unsigned>(len));
+#elif BUILDFLAG(IS_STARBOARD)


write is supported in Starboard, why is this needed?

andrewsavage1 · 2025-01-14T15:24:42Z

third_party/musl/include/pthread.h

@@ -1,11 +1,11 @@
 #ifndef _PTHREAD_H
 #define _PTHREAD_H

-#if defined(STARBOARD)
+// #if defined(STARBOARD)


Should this be changed before submission?

y4vor · 2025-01-15T20:24:48Z

base/BUILD.gn

 # Determines whether libevent should be dep.
-dep_libevent = !is_fuchsia && !is_win && !is_mac && !is_nacl
+if (is_cobalt && is_cobalt_hermetic_build) {
+  dep_libevent = false


+1 to add it to the args.gn

y4vor · 2025-01-15T20:34:26Z

third_party/musl/include/pthread.h


-#include "third_party/musl/src/starboard/pthread/pthread.h"
+#if (BUILDFLAG(IS_STARBOARD) && !defined(_LIBCPP_HAS_MUSL_LIBC))


Remove the _LIBCPP_HAS_MUSL_LIBC condition. It is a libcxx to flag. This header file is inside musl.

y4vor · 2025-01-15T20:42:24Z

third_party/abseil-cpp/absl/base/BUILD.gn

@@ -3,6 +3,9 @@
 # found in the LICENSE file.

 import("//third_party/abseil-cpp/absl.gni")
+if (is_cobalt) {


why some targets need to import the modular_variables.gni and other don't?
Ideally some script early on import this file for all targets.

it's for the is_hermetic_build variable which is temporary—Niranjan and I talked about using is_starboard in the long term
But generally we don't want variables to be global, it's essentially the same as the c++ iwyu concept

y4vor · 2025-01-15T20:44:23Z

third_party/abseil-cpp/absl/base/internal/raw_logging.cc

@@ -27,6 +27,11 @@
 #include "absl/base/internal/errno_saver.h"
 #include "absl/base/log_severity.h"

+#include "build/build_config.h"
+
+#if BUILDFLAG(IS_STARBOARD) && defined(_LIBCPP_HAS_MUSL_LIBC)


We wouldn't have non-hermetic IS_STARBOARD. Is this a transitional step? Also _LIBCPP_HAS_MUSL_LIBC is a variable only for libcxx. It doesn't make sense to use it in any other modules.

y4vor · 2025-01-15T20:49:00Z

starboard/build/config/modular/BUILD.gn

  defines = [
-    # Ensure that the Starboardized __external_threading file is included.
-    "_LIBCPP_HAS_THREAD_API_EXTERNAL",
+    # # Ensure that the Starboardized __external_threading file is included.


cleanup those?

y4vor · 2025-01-15T20:51:35Z

buildtools/third_party/libc++abi/BUILD.gn

@@ -21,6 +24,10 @@ source_set("libc++abi") {
    deps = [ "//buildtools/third_party/libunwind" ]
  }

+  if (is_cobalt && is_cobalt_hermetic_build) {


why do we need this dependency here? All targets would be using musl.

y4vor · 2025-01-15T20:52:10Z

buildtools/third_party/libc++/BUILD.gn

@@ -115,6 +120,11 @@ target(_libcxx_target_type, "libc++") {
    "trunk/src/verbose_abort.cpp",
  ]

+  if (is_cobalt && is_cobalt_hermetic_build) {


we need atomics, what is he build error?

y4vor · 2025-01-15T20:54:10Z

base/system/sys_info_posix.cc

@@ -35,7 +35,7 @@
 #include <sys/statvfs.h>
 #endif

-#if BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS)
+#if (BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS)) && !(BUILDFLAG(IS_STARBOARD) && defined(_LIBCPP_HAS_MUSL_LIBC))


don't use _LIBCPP_HAS_MUSL_LIBC as this is a libcxx flag only.

y4vor · 2025-01-15T21:01:04Z

base/debug/stack_trace_posix.cc

  // See: https://man7.org/linux/man-pages/man2/exit_group.2.html
  syscall(SYS_exit_group, EXIT_FAILURE);
+#else
+// No clue how to substitue , ask yavor


The call seem to be needed only as a workaround for AMD Linux shutdowns of the GPU process. We can probably just call SbSystemBreakIntoDebugger or abort.

niranjanyardi · 2025-01-16T01:10:07Z

It looks like this isn't changing any DEPS repos to subtrees—is it no longer needed?

currently, im modifying a file in boringssl - so my understanding is that repo needs to be added

niranjanyardi requested review from a team as code owners December 14, 2024 04:17

niranjanyardi requested a review from johnxwork December 14, 2024 04:17

niranjanyardi marked this pull request as draft December 14, 2024 04:17

niranjanyardi force-pushed the build_hermeticaly branch from 0e0c67d to a61fe7b Compare December 14, 2024 04:19

niranjanyardi changed the title ~~Add buildtools/third_party/libc++/trunk~~ Build base/allocator/partition_allocator:partition_alloc hermetically Dec 14, 2024

niranjanyardi removed the request for review from johnxwork December 14, 2024 04:24

niranjanyardi force-pushed the build_hermeticaly branch from a61fe7b to abaf11c Compare December 17, 2024 09:50

niranjanyardi changed the title ~~Build base/allocator/partition_allocator:partition_alloc hermetically~~ Build nplb hermetically Dec 17, 2024

niranjanyardi requested review from y4vor, kaidokert and andrewsavage1 December 17, 2024 09:52

y4vor reviewed Dec 17, 2024

View reviewed changes

niranjanyardi commented Dec 17, 2024

View reviewed changes

build/config/c++/BUILD.gn Outdated Show resolved Hide resolved

andrewsavage1 reviewed Dec 17, 2024

View reviewed changes

niranjanyardi mentioned this pull request Dec 17, 2024

Build nplb hermetically #4597

Closed

niranjanyardi force-pushed the build_hermeticaly branch from abaf11c to efba4d2 Compare January 11, 2025 02:25

niranjanyardi commented Jan 14, 2025

View reviewed changes

niranjanyardi force-pushed the build_hermeticaly branch from 4e14f7c to d8354f7 Compare January 14, 2025 01:33

niranjanyardi marked this pull request as ready for review January 14, 2025 02:28

andrewsavage1 reviewed Jan 14, 2025

View reviewed changes

niranjanyardi added 7 commits January 14, 2025 18:40

Build nplb hermetically

7fb4c7a

Changes to cleanup

95ab446

Modify openssl.h

f823697

Fix precommit run

79c2f65

Fix issues while building chromium targets

8a886c3

Fix GN, compiler build errors

977982c

Fix build errors in chromium linux builds

50a4d3e

niranjanyardi added 9 commits January 14, 2025 18:40

Remove unnecessary conditions

9453f18

Fix build errors for all CI builds

639565f

More changes to get builds working

05e69c4

Renable enable_message_pump_epoll

3177321

Add is_cobalt_hermetic_build

e193bfe

More semantic cleanup

215fd79

Cleanup starboard/BUILD.gn

81a7c94

Remove starboard_group dep on libc++, libc++abi

3373730

Fix base_unittests

8c9999a

niranjanyardi force-pushed the build_hermeticaly branch from be31396 to 8c9999a Compare January 15, 2025 02:40

y4vor reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build nplb hermetically #4587

Build nplb hermetically #4587

niranjanyardi commented Dec 14, 2024 •

edited

Loading

niranjanyardi Jan 14, 2025

niranjanyardi Jan 14, 2025 •

edited

Loading

andrewsavage1 Jan 14, 2025

y4vor Jan 15, 2025

andrewsavage1 left a comment

andrewsavage1 Jan 14, 2025

andrewsavage1 Jan 14, 2025

andrewsavage1 Jan 14, 2025

andrewsavage1 Jan 14, 2025

andrewsavage1 Jan 14, 2025

andrewsavage1 Jan 15, 2025

y4vor Jan 15, 2025

andrewsavage1 Jan 14, 2025

andrewsavage1 Jan 14, 2025

andrewsavage1 Jan 14, 2025

niranjanyardi Jan 14, 2025

y4vor Jan 15, 2025

andrewsavage1 Jan 14, 2025

andrewsavage1 Jan 14, 2025

y4vor Jan 15, 2025

y4vor Jan 15, 2025

y4vor Jan 15, 2025

andrewsavage1 Jan 15, 2025

y4vor Jan 15, 2025

y4vor Jan 15, 2025

y4vor Jan 15, 2025

y4vor Jan 15, 2025

y4vor Jan 15, 2025

y4vor Jan 15, 2025

niranjanyardi commented Jan 16, 2025


		#include "third_party/musl/src/starboard/pthread/pthread.h"
		#if (BUILDFLAG(IS_STARBOARD) && !defined(_LIBCPP_HAS_MUSL_LIBC))

Build nplb hermetically #4587

Are you sure you want to change the base?

Build nplb hermetically #4587

Conversation

niranjanyardi commented Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

niranjanyardi Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsavage1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niranjanyardi commented Jan 16, 2025

niranjanyardi commented Dec 14, 2024 •

edited

Loading

niranjanyardi Jan 14, 2025 •

edited

Loading