GH-51834: [C++] Reduce ARROW_ASSIGN_OR_RAISE stack consumption with gcc/clang

[taepper]

Rationale for this change

@pitrou found in investigations that repeated uses of ARROW_ASSIGN_OR_RAISE can lead to very large stack consumptions.

What changes are included in this PR?

This PR adds a new definition of the ARROW_ASSIGN_OR_RAISE macro, which avoids intermediate results using a gcc/clang compiler extension.

Are these changes tested?

No new feature, all tests still pass.

Are there any user-facing changes?

Yes. The ARROW_ASSIGN_OR_RAISE macro is a public interface and now has a breaking change:

If lhs is a declaration, it will already be in scope for the body of the right-hand-side. This lead to one breakage in the arrow code-base:

    ARROW_ASSIGN_OR_RAISE(auto options,
                          GetReadOptions(*reader->schema(), *self, *options));

which needed manual fixing. This could also break users of the C++ library

Was AI used for this PR?

In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.

PR code and description written by:

• Human
• AI

Reviewed before submission by:

• Human
• AI
• Not reviewed

• GitHub Issue: [C++] Excessive stack consumption with ARROW_ASSIGN_OR_RAISE #51834

[github-actions]

⚠️ GitHub issue #51834 has been automatically assigned in GitHub to PR creator.

[createcentury]

The gcc/clang specialization looks good to me.

I checked the two things this expansion could have quietly broken, and both come out clean:

• Shadowing. Moving lhs to the outside of the statement expression changes name
  lookup inside the initializer, which is what the file_ipc.cc rename is about. I went
  through the rest of cpp/src for the same pattern and that is the only site, so nothing
  else needs renaming.
• Lifetime. The intermediate Result now dies at the end of the statement rather than
  the enclosing scope. Nothing binds it by reference — there are no auto&& or auto& left
  hand sides — and the std::string_view ones view into storage owned elsewhere, so they're
  unaffected.

clang at -O0 stays where it was, as your table shows, but killing the growth in gcc -O0
and in both release builds covers most of the pain.

[HuaHuaY]

While discussing this PR with my LLM, I unexpectedly discovered an issue. The GCC documentation https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html contains the following statement:

In G++, the result value of a statement expression undergoes array and function pointer decay, and is returned by value to the enclosing expression.

This extended syntax constructs a temporary value. Issues arise when encountering const Result<T>. Consider the following two code snippets:

arrow::Status Repro() {
  const arrow::Result<std::string> result = std::string(128, 'x');
  ARROW_ASSIGN_OR_RAISE(std::string_view view, result);
  std::printf("%c\n", view.front());
  return arrow::Status::OK();
}

In the first code snippet, the string_view references a temporary value created via extended syntax, resulting in a dangling reference.

const arrow::Result<std::unique_ptr<int>> result = std::make_unique<int>(42);
ARROW_ASSIGN_OR_RAISE(const auto& value, result);

The second code snippet fails to compile because unique_ptr cannot be copied via value semantics.

There is a TRY macro for C++23's std::expected on Reddit that is similar to our implementation, and I found that it encounters the same issue. I consider this issue acceptable.

[taepper]

In the first code snippet, the string_view references a temporary value created via extended syntax, resulting in a dangling reference.

I think you are correct that our Warnings do not currently cover this case explicitly:

/// WARNING: ARROW_ASSIGN_OR_RAISE `std::move`s its right operand. If you have
/// an lvalue Result which you *don't* want to move out of cast appropriately.
///
/// WARNING: ARROW_ASSIGN_OR_RAISE is not a single expression; it will not
/// maintain lifetimes of all temporaries in `rexpr` (e.g.
/// `ARROW_ASSIGN_OR_RAISE(auto x, MakeTemp().GetResultRef());`
/// will most likely segfault)!

If I understand correctly, the issue arises in your example, because the std::move(rexpr) cannot move the _const_ Result<std::string> and will therefore copy the string to the temporary which will not outlive the statement expression.

IMO this is implicitly covered by this statement

ARROW_ASSIGN_OR_RAISE is not a single expression; it will not maintain lifetimes of all temporaries in rexpr

Because the caller provides a const Result it is not unexpected that the evaluation will perform a copy of rexpr to a temporary. And the doc already warns regarding this temporary's lifetime

[rok]

edit: good call.

[taepper]

Well, because we use auto here, we got a compile-time error. Otherwise, rexpr would use the lhs's declaration as an uninitialized object.

Even when keeping in mind that

auto options = GetReadOptions(*reader->schema(), *self, *options)).ValueUnsafe();

would obviously already have been broken before, this breakage is definitely something to consider, given that it can introduce potentially subtle bugs in broken but working consumers of the macro

[pitrou]

@github-actions crossbow submit -g cpp

[github-actions]

Revision: f96d9fa

Submitted crossbow builds: ursacomputing/crossbow @ actions-9760cf6651

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-debian-13-cpp-amd64
test-debian-13-cpp-i386
test-debian-experimental-cpp-gcc-15
test-fedora-42-cpp
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-bundled
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

[pitrou]

I'm going to merge now. Thanks @taepper for submitting this PR!