src/libutil-tests/cxa-throw.cc (1)

6-35: Good coverage of std::logic_error family, but missing negative test.

The death tests comprehensively cover std::logic_error and its standard subclasses. However, there's no test verifying that non-logic-error exceptions (e.g., std::runtime_error) are not aborted and can be caught normally. This would validate that the mechanism is selective.

Suggested additional test

TEST(CxaThrow, doesNotCatchRuntimeError)
{
    // Verify that non-logic_error exceptions are not intercepted
    bool caught = false;
    try {
        throw std::runtime_error("test");
    } catch (const std::runtime_error &) {
        caught = true;
    }
    ASSERT_TRUE(caught);
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/libutil-tests/cxa-throw.cc` around lines 6 - 35, Add a negative test
named TEST(CxaThrow, doesNotCatchRuntimeError) that verifies non-logic_error
exceptions are not intercepted: in the test throw std::runtime_error("test")
inside a try/catch, set a local bool (e.g., caught) to true in the catch(const
std::runtime_error&) block, and ASSERT_TRUE(caught) at the end; this complements
the existing TEST(CxaThrow,... ) cases (catchesLogicErrorFromStdlib,
catchesLogicError, catchesOutOfRange, catchesInvalidArgument,
catchesDomainError, catchesLengthError) to ensure runtime_error is handled
normally rather than causing abortion.

nix-meson-build-support/common/cxa-throw/wrap-cxa-throw.cc (1)

24-32: Consider adding [[noreturn]] attribute.

Same as the interpose version - __wrap___cxa_throw never returns normally.

Proposed fix

-extern "C" void __wrap___cxa_throw(void * exc, std::type_info * tinfo, void (*dest)(void *))
+extern "C" [[noreturn]] void __wrap___cxa_throw(void * exc, std::type_info * tinfo, void (*dest)(void *))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nix-meson-build-support/common/cxa-throw/wrap-cxa-throw.cc` around lines 24 -
32, The wrapper function __wrap___cxa_throw never returns, so add the
[[noreturn]] attribute to its declaration/definition (extern "C" [[noreturn]]
void __wrap___cxa_throw(...)) to match the interpose version; keep the existing
logic (is_logic_error check, CXA_THROW_ON_LOGIC_ERROR(), call to
__real___cxa_throw(exc, tinfo, dest) and the __builtin_unreachable()) but mark
the function [[noreturn]] so callers and the compiler know it does not return.

nix-meson-build-support/common/cxa-throw/meson.build (1)

18-24: Hardcoded -ldl in test may cause false negatives on musl/BSD systems.

Line 20 uses args : [ '-ldl' ] unconditionally, but on systems like musl libc or some BSDs, dlsym is in libc and -ldl doesn't exist. This could cause the interpose test to fail compilation even when interposition would work, incorrectly falling through to the wrap approach.

Consider using cxx.find_library('dl', required : false) for the test as well:

Proposed fix

+dl_dep = cxx.find_library('dl', required : false)
+dl_args = dl_dep.found() ? [ '-ldl' ] : []
+
 can_interpose_cxa_throw_result = cxx.run(
   can_interpose_cxa_throw_test_code,
-  args : [ '-ldl' ],
+  args : dl_args,
   include_directories : include_directories('.'),
   name : 'can interpose __cxa_throw (catches libstdc++ throws)',
 )

Or use the dependency directly if cxx.run() supports it.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nix-meson-build-support/common/cxa-throw/meson.build` around lines 18 - 24,
The test for interposing __cxa_throw uses a hardcoded args : [ '-ldl' ] which
can break on musl/BSD where libdl isn't separate; update the cxx.run invocation
that produces can_interpose_cxa_throw_result to use a discovered optional dl
library instead: call cxx.find_library('dl', required : false) (or the project's
equivalent) and pass that library/dependency into cxx.run (or its deps/args)
only when found, so can_interpose_cxa_throw_result.compiled() reflects the true
ability to link dlsym without forcing -ldl; adjust code that sets
can_interpose_cxa_throw to use the new result unchanged.

nix-meson-build-support/common/cxa-throw/interpose-cxa-throw.cc (1)

25-37: Consider adding [[noreturn]] attribute.

The __cxa_throw function never returns normally (it either aborts or throws). Adding the [[noreturn]] attribute would allow the compiler to better optimize callers and provide better diagnostics.

Proposed fix

-extern "C" void __cxa_throw(void * exc, std::type_info * tinfo, void (*dest)(void *))
+extern "C" [[noreturn]] void __cxa_throw(void * exc, std::type_info * tinfo, void (*dest)(void *))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nix-meson-build-support/common/cxa-throw/interpose-cxa-throw.cc` around lines
25 - 37, Add the C++ [[noreturn]] attribute to the interposed function and
matching function-pointer typedef so the compiler knows __cxa_throw never
returns; specifically modify the extern "C" void __cxa_throw(...) definition to
be extern "C" [[noreturn]] void __cxa_throw(void *exc, std::type_info *tinfo,
void (*dest)(void *)) and update the cxa_throw_type alias (the type used for
orig) to a pointer-to-[[noreturn]] function signature so the orig lookup and
subsequent call have the noreturn annotation as well.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

nix-meson-build-support/common/cxa-throw/meson.build (1)

54-72: Consider portability of linker flags.

The --wrap and -u linker flags are GNU ld / lld specific. This should be fine since the feature test will naturally fail on systems with incompatible linkers (e.g., macOS ld64), correctly disabling the feature. Just noting for awareness.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nix-meson-build-support/common/cxa-throw/meson.build` around lines 54 - 72,
The linker flags '-Wl,--wrap=__cxa_throw' and '-Wl,-u,__wrap___cxa_throw' are
GNU/LLD-specific; update the feature gating around
wrap_cxa_throw_args/can_wrap_cxa_throw_result/cxa_throw_dep so we only attempt
the cxx.run test and create the static_library/declare_dependency when the
detected linker/platform supports those flags (e.g., skip on Darwin or check
cc.get_id()/compiler.get_linker_name() or host_machine.system() before using
wrap_cxa_throw_args). Ensure the platform/linker check is performed before
defining wrap_cxa_throw_args or invoking cxx.run so the test won’t run with
incompatible linkers.

nix-meson-build-support/common/cxa-throw/meson.build (1)

12-13: Prefer a defined stdlib throw here unless the null-string backtrace is the point.

std::string(nullptr) is undefined by the C++ API and only throws here because libstdc++ adds a check. If the goal is just to exercise a stdlib std::logic_error path, a defined throw site such as std::string{}.at(1) is much less brittle.

Example change

-    const char * volatile p = nullptr;
-    std::string s(p);
+    std::string s;
+    s.at(1);

-      const char * volatile p = nullptr;
-      std::string s(p);
+      std::string s;
+      s.at(1);

Also applies to: 48-49

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nix-meson-build-support/common/cxa-throw/meson.build` around lines 12 - 13,
Replace the undefined call std::string s(p) (where p is declared as const char *
volatile p = nullptr) with a defined standard-library throw site; for example,
remove the null-pointer construction and use a bounds-checked string access like
std::string{}.at(1) to reliably trigger a std::out_of_range (or explicitly throw
a std::logic_error if that exact exception type is required). Apply the same
change at the other occurrence referenced (the code around the symbols at the
second occurrence corresponding to the previous std::string(nullptr) usage on
lines 48-49).

src/libutil-tests/cxa-throw.cc (1)

6-35: Tighten this suite around both the abort and pass-through paths.

These tests only prove that some logic-error paths die. They do not cover the inverse requirement from the PR (std::runtime_error and other non-logic_error throws must still propagate normally), and ASSERT_DEATH(..., "") also accepts deaths that are not specifically abort(). An over-broad matcher or a different fatal path would still go green here.

Example tightening

+#include <csignal>

-    ASSERT_DEATH({ throw std::logic_error("test"); }, "");
+    ASSERT_EXIT({ throw std::logic_error("test"); },
+        ::testing::KilledBySignal(SIGABRT), "");

+TEST(CxaThrow, ignoresRuntimeError)
+{
+    EXPECT_THROW([] { throw std::runtime_error("test"); }(), std::runtime_error);
+}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/libutil-tests/cxa-throw.cc` around lines 6 - 35, Replace the broad
ASSERT_DEATH(...) checks in the CxaThrow tests with precise checks that the
process aborts via SIGABRT (use gtest's KilledBySignal(SIGABRT) / a regex that
matches "Aborted") for the logic_error variants (tests named
CxaThrow::catchesLogicError, catchesOutOfRange, catchesInvalidArgument,
catchesDomainError, catchesLengthError and the stdlib string-nullptr case), and
add a new test (e.g., CxaThrow::catchesRuntimeErrorPropagates) that uses
ASSERT_THROW({ throw std::runtime_error("test"); }, std::runtime_error) to
verify std::runtime_error and other non-logic_error exceptions propagate instead
of causing abort; update only the assertions in those tests (no behavior
changes).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

nix-meson-build-support/common/cxa-throw/is-logic-error.hh (1)

18-22: ⚠️ Potential issue | 🟠 Major

is_logic_error still misses multiple-inheritance RTTI paths.

Line 18 only walks __si_class_type_info; a std::logic_error-derived type using multiple inheritance can bypass detection. This was already raised earlier and appears unresolved.

Proposed fix

 static bool is_logic_error(const std::type_info * tinfo)
 {
     if (*tinfo == typeid(std::logic_error))
         return true;

     auto * si = dynamic_cast<const __cxxabiv1::__si_class_type_info *>(tinfo);
     if (si)
         return is_logic_error(si->__base_type);
+
+    auto * vmi = dynamic_cast<const __cxxabiv1::__vmi_class_type_info *>(tinfo);
+    if (vmi) {
+        for (unsigned i = 0; i < vmi->__base_count; ++i) {
+            if (is_logic_error(vmi->__base_info[i].__base_type))
+                return true;
+        }
+    }

     return false;
 }

#!/bin/bash
set -euo pipefail
rg -n '__si_class_type_info|__vmi_class_type_info|is_logic_error' nix-meson-build-support/common/cxa-throw/is-logic-error.hh -C3

nix-meson-build-support/common/cxa-throw/interpose-cxa-throw.cc (1)

11-19: Add a non-logic_error passthrough regression test.

This forwarding path is core to the “don’t abort other exceptions” objective, but current tests only cover death on logic_error variants (src/libutil-tests/cxa-throw.cc, Lines 1-35). Please add one test that throws/catches something like std::runtime_error and verifies no abort.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nix-meson-build-support/common/cxa-throw/interpose-cxa-throw.cc` around lines
11 - 19, Add a regression test that verifies non-logic_error exceptions are
forwarded and do not trigger abort: create a test (e.g.,
cxa_throw_passthrough_non_logic_error) that throws a std::runtime_error (or
another non-logic_error type) so the interpose path in is_logic_error(...) is
exercised, ensure the exception is caught by the test (or the child process
exits normally) and assert no abort occurred; this validates the orig
(cxa_throw_type) dlsym(RTLD_NEXT, "__cxa_throw") forwarding path and that
abort_on_exception(exc, tinfo) is not invoked for non-logic_error types.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes