Support -uptr mode in xdrpp by graydon · Pull Request #5467 · stellar/stellar-core

graydon · 2026-09-21T23:44:48Z

This adopts the xdrpp changes in https://github.com/xdrpp/xdrpp/tree/cxx20-uptr-hybrid and enables it at 32 byte default threshold for out-of-line allocations; it also adds --enable-xdrpp-uptr=N flag to configure this threshold.

Copilot

Copilot review overview

🟡 Changes recommended

Generated headers can remain stale when toggling the option, and the new build mode lacks automated coverage.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 1

Open (2)

Regenerate headers when --enable-xdrpp-uptr changes · New
Add CI coverage for --enable-xdrpp-uptr mode · New

What changed in this PR

Adds optional pointer-backed XDR union generation and updates XDR query validation for the newer xdrpp API.

Changes:

Adds --enable-xdrpp-uptr.
Passes -uptr during XDR header generation.
Uses union metadata to validate fields without copying unions.

File	Description
`configure.ac`	Defines the optional uptr configuration flag.
`src/Makefile.am`	Passes configured flags to `xdrc`.
`src/util/xdrquery/XDRFieldResolver.h`	Adapts union-field validation to the new API.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Copilot review overview

🔵 Needs a closer look

Generated headers are not invalidated when the mode changes, and the new build variant lacks CI coverage.

Review effort: Balanced
Findings: 1 · 1

Open (2)

Regenerate headers when --enable-xdrpp-uptr changes
Add CI coverage for --enable-xdrpp-uptr mode

Copilot

Copilot review overview

🔵 Needs a closer look

The -uptr path lacks CI coverage, incremental builds may retain stale headers, and a required standard header is not directly included.

Review effort: Balanced
Findings: 1 · 1

Open (2)

Regenerate headers when --enable-xdrpp-uptr changes
Add CI coverage for --enable-xdrpp-uptr mode

Copilot

Copilot review overview

🔵 Needs a closer look

The generated-header dependency and CI coverage issues must be addressed before approval.

Review effort: Balanced
Findings: 1 · 1

Open (2)

Regenerate headers when --enable-xdrpp-uptr changes
Add CI coverage for --enable-xdrpp-uptr mode

Copilot

Copilot review overview

🟡 Changes recommended

The build rule has a critical clean-build failure, and the new mode lacks CI coverage.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 1

Open (2)

Missing output directory creation breaks clean out-of-tree builds · New
Add CI coverage for --enable-xdrpp-uptr mode

Resolved since last review (1)

Regenerate headers when --enable-xdrpp-uptr changes

Copilot

Copilot review overview

🟡 Changes recommended

The missing include and lack of CI coverage for the new supported mode must be addressed.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

Include <string_view> or avoid direct string_view usage · New

Resolved since last review (2)

Missing output directory creation breaks clean out-of-tree builds
Add CI coverage for --enable-xdrpp-uptr mode

Copilot

Copilot review overview

🟡 Changes recommended

The default configuration selects an invalid empty threshold, and the new storage modes lack CI coverage.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 1

Open (2)

Default configure emits an invalid empty uptr threshold · New
Add CI coverage for all-indirect and threshold uptr modes · New

Resolved since last review (1)

Include <string_view> or avoid direct string_view usage

Copilot

Copilot review overview

🟡 Changes recommended

Empty option validation remains faulty, and the new union-storage modes are not exercised by CI.

Review effort: Balanced
Findings: 2

Open (2)

Reject empty --enable-xdrpp-uptr value during configuration · New
Add CI coverage for all-indirect and threshold uptr modes

Resolved since last review (1)

Default configure emits an invalid empty uptr threshold

Copilot

Copilot review overview

🟡 Changes recommended

Dependency incompatibilities block builds, union validation accepts invalid paths, and unchanged headers repeatedly regenerate.

Review effort: Balanced
Findings: 1 · 3

Open (4)

Align xdrpp dependency with required xdrc interfaces · New
Prevent repeated regeneration when unchanged headers retain old timestamps · New
Stop traversal after matching a union arm · New
Add CI coverage for all-indirect and threshold uptr modes

Resolved since last review (1)

Reject empty --enable-xdrpp-uptr value during configuration

Copilot

Copilot review overview

🟡 Changes recommended

Parallel recovery of missing generated headers can launch competing generators and fail the build.

Review effort: Balanced
Findings: 1 · 1

Open (2)

Avoid parallel regeneration races for missing headers · New
Add CI coverage for all-indirect and threshold uptr modes

Resolved since last review (3)

Align xdrpp dependency with required xdrc interfaces
Stop traversal after matching a union arm
Prevent repeated regeneration when unchanged headers retain old timestamps

Copilot

Copilot review overview

🔵 Needs a closer look

The default union-storage change needs human review backed by build, runtime, and performance validation against the adopted xdrpp revision.

Review effort: Balanced
Findings: 1

Open (1)

Add CI coverage for all-indirect and threshold uptr modes

Resolved since last review (1)

Avoid parallel regeneration races for missing headers

Copilot

Copilot review overview

🔵 Needs a closer look

The default changes storage layouts across core XDR types and needs full build and runtime validation before human approval.

Review effort: Balanced
Findings: None

Resolved since last review (1)

Add CI coverage for all-indirect and threshold uptr modes

Copilot

Copilot review overview

🔵 Needs a closer look

Default-on changes to core XDR storage layouts require human confirmation of dependency compatibility, runtime correctness, and performance.

Review effort: Balanced
Findings: None

Copilot

Copilot review overview

🔵 Needs a closer look

Default union-layout changes require human validation, and clean Visual Studio builds have an unresolved missing-header issue.

Review effort: Balanced
Findings: 1

Open (1)

Add Visual Studio generation rules for missing contract headers · New

 #include "test/test.h"
+#include "xdr/Stellar-SCP.h"
+#include "xdr/Stellar-contract-config-setting.h"
+#include "xdr/Stellar-contract-env-meta.h"


sisuresh

--enable-xdrpp-uptr now defaults to 32 (configure.ac:544), contrary to the description. Every LedgerEntry.data arm is >32 bytes, and the new xdrpp allocates indirect arms on default-construct, copy and move. Replaying the LiveBucket load path over 1k accounts gave ~4k allocations vs 2 and was >2x slower. I'd keep it opt-in until there's an apply-load/catchup benchmark.
CI-private was skipped on this PR and the docker builds use --disable-tests, so none of the BUILD_TESTS code (dump-xdr-unions, the new resolver test, fuzz/autocheck utils) has been compiled or run in CI.
CommandLine.cpp:55-58 includes Stellar-contract-env-meta.h, Stellar-contract-meta.h and Stellar-exporter.h, which the VS project doesn't generate, so clean VS builds will fail.
Interrupting make during xdr-stamp (Makefile.am:127-145) leaves a truncated header and .h.tmp, since the backup is only restored when xdrc fails normally.

Copilot

🟡 Changes recommended

The diagnostic omits contract-spec unions, and the globally enabled allocation change lacks required performance evidence.

3 open findings

Add Visual Studio generation rules for missing contract headers
Benchmark the 32-byte default before enabling it globally · New
Include Stellar contract spec XDR unions in layout analysis · New

🧠 Review effort: Balanced

+        [Indirect xdrpp union arms larger than BYTES (default 32); pass
+         --enable-xdrpp-uptr to indirect every arm, or --disable-xdrpp-uptr
+         to store every arm inline]),
+    [], [enable_xdrpp_uptr=32])


+        dumpUnionLayouts(xdr::xdr_union_types_Stellar_SCP{}, totals);
+        dumpUnionLayouts(xdr::xdr_union_types_Stellar_transaction{}, totals);
+        dumpUnionLayouts(xdr::xdr_union_types_Stellar_types{}, totals);


Copilot AI balanced review requested due to automatic review settings September 21, 2026 23:44

Copilot started reviewing on behalf of graydon September 21, 2026 23:45

View session

Copilot AI reviewed Sep 21, 2026

View reviewed changes

Comment thread src/Makefile.am Outdated

Comment thread configure.ac Outdated

graydon force-pushed the xdrpp-cxx20-uptr branch from 24fa527 to b427129 Compare September 22, 2026 05:28

Copilot AI review requested due to automatic review settings September 22, 2026 05:28

Copilot started reviewing on behalf of graydon September 22, 2026 05:28

View session

Copilot AI reviewed Sep 22, 2026

View reviewed changes

graydon force-pushed the xdrpp-cxx20-uptr branch from b427129 to e7b9b5b Compare September 22, 2026 19:47

Copilot AI review requested due to automatic review settings September 22, 2026 19:47

Copilot started reviewing on behalf of graydon September 22, 2026 19:48

View session

Copilot AI reviewed Sep 22, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 22, 2026 20:11

graydon force-pushed the xdrpp-cxx20-uptr branch from e7b9b5b to dcf658c Compare September 22, 2026 20:11

Copilot started reviewing on behalf of graydon September 22, 2026 20:11

View session

Copilot AI reviewed Sep 22, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 23, 2026 00:51

Copilot started reviewing on behalf of graydon September 23, 2026 00:51

View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

Comment thread src/Makefile.am

Copilot AI review requested due to automatic review settings September 23, 2026 05:43

graydon force-pushed the xdrpp-cxx20-uptr branch from 0a118b5 to 72eaedb Compare September 23, 2026 05:43

Copilot started reviewing on behalf of graydon September 23, 2026 05:44

View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

Comment thread src/util/xdrquery/XDRFieldResolver.h

Copilot AI review requested due to automatic review settings September 26, 2026 00:16

Copilot started reviewing on behalf of graydon September 26, 2026 00:17

View session

Copilot AI reviewed Sep 26, 2026

View reviewed changes

Comment thread configure.ac Outdated

Comment thread configure.ac Outdated

Copilot AI review requested due to automatic review settings September 26, 2026 00:58

graydon force-pushed the xdrpp-cxx20-uptr branch from cb4fe81 to b5d9614 Compare September 26, 2026 00:58

Copilot started reviewing on behalf of graydon September 26, 2026 00:59

View session

Copilot AI reviewed Sep 26, 2026

View reviewed changes

Comment thread configure.ac Outdated

Copilot AI balanced review requested due to automatic review settings October 2, 2026 22:37

graydon force-pushed the xdrpp-cxx20-uptr branch from b5d9614 to dfc717a Compare October 2, 2026 22:37

Copilot started reviewing on behalf of graydon October 2, 2026 22:37

View session

Copilot AI reviewed Oct 2, 2026

View reviewed changes

Comment thread configure.ac Outdated

Comment thread src/Makefile.am Outdated

Comment thread src/util/xdrquery/XDRFieldResolver.h Outdated

Copilot AI balanced review requested due to automatic review settings October 2, 2026 23:43

graydon force-pushed the xdrpp-cxx20-uptr branch from dfc717a to a7819c9 Compare October 2, 2026 23:43

Copilot started reviewing on behalf of graydon October 2, 2026 23:44

View session

Copilot AI reviewed Oct 2, 2026

View reviewed changes

Comment thread src/Makefile.am Outdated

Copilot AI balanced review requested due to automatic review settings October 3, 2026 00:06

Copilot started reviewing on behalf of graydon October 3, 2026 00:07

View session

Copilot AI reviewed Oct 3, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 3, 2026 00:10

graydon force-pushed the xdrpp-cxx20-uptr branch from 1e19beb to b22c7aa Compare October 3, 2026 00:10

Copilot started reviewing on behalf of graydon October 3, 2026 00:11

View session

Copilot AI reviewed Oct 3, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 3, 2026 00:24

graydon force-pushed the xdrpp-cxx20-uptr branch from b22c7aa to 5a92319 Compare October 3, 2026 00:24

Copilot started reviewing on behalf of graydon October 3, 2026 00:24

View session

Copilot AI reviewed Oct 3, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 6, 2026 02:55

graydon force-pushed the xdrpp-cxx20-uptr branch from 5a92319 to 488e91a Compare October 6, 2026 02:55

Copilot started reviewing on behalf of graydon October 6, 2026 02:55

View session

Copilot AI reviewed Oct 6, 2026

View reviewed changes

Comment thread src/main/CommandLine.cpp

#include "test/test.h"

#include "xdr/Stellar-SCP.h"

#include "xdr/Stellar-contract-config-setting.h"

#include "xdr/Stellar-contract-env-meta.h"

sisuresh reviewed Oct 6, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 9, 2026 06:35

graydon force-pushed the xdrpp-cxx20-uptr branch from 488e91a to e5e6df3 Compare October 9, 2026 06:35

Copilot started reviewing on behalf of graydon October 9, 2026 06:35

View session

Copilot AI reviewed Oct 9, 2026

View reviewed changes

graydon force-pushed the xdrpp-cxx20-uptr branch from e5e6df3 to 060153d Compare October 23, 2026 20:40

Support pointer-backed xdrpp unions

0feefb4

graydon force-pushed the xdrpp-cxx20-uptr branch from 060153d to 0feefb4 Compare October 23, 2026 21:03

Conversation

graydon commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

graydon commented Sep 21, 2026 •

edited

Loading