Skip to content

C++23: Replace variant<T, string> with expected #1074

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
elazarg wants to merge 2 commits into
base: main
Choose a base branch
from
Open

C++23: Replace variant<T, string> with expected #1074

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e0da22e
replace variant<T, string> with expected
elazarg Apr 15, 2026
4ed60f3
avoid leaking C++23 to public API
elazarg Apr 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/ir/unmarshal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Prevail Verifier contributors.
// SPDX-License-Identifier: MIT
#include <cassert>
#include <expected>
#include <iostream>
#include <string>
#include <vector>
Expand Down Expand Up @@ -782,8 +783,7 @@ struct Unmarshaller {
}
}

vector<LabeledInstruction> unmarshal(vector<EbpfInst> const& insts,
const prevail::ebpf_verifier_options_t& options) {
vector<LabeledInstruction> unmarshal(vector<EbpfInst> const& insts, const ebpf_verifier_options_t& options) {
options.validate();
subprogram_stack_size = options.subprogram_stack_size;
vector<LabeledInstruction> prog;
Expand Down Expand Up @@ -882,20 +882,20 @@ struct Unmarshaller {
}
};

std::variant<InstructionSeq, std::string> unmarshal(const RawProgram& raw_prog, vector<vector<string>>& notes,
const prevail::ebpf_verifier_options_t& options) {
std::expected<InstructionSeq, std::string> unmarshal(const RawProgram& raw_prog, vector<vector<string>>& notes,
const ebpf_verifier_options_t& options) {
thread_local_program_info = raw_prog.info;
try {
return Unmarshaller{notes, raw_prog.info}.unmarshal(raw_prog.prog, options);
} catch (InvalidInstruction& arg) {
std::ostringstream ss;
ss << arg.pc << ": " << arg.what() << "\n";
return ss.str();
return std::unexpected(ss.str());
}
}

std::variant<InstructionSeq, std::string> unmarshal(const RawProgram& raw_prog,
const prevail::ebpf_verifier_options_t& options) {
std::expected<InstructionSeq, std::string> unmarshal(const RawProgram& raw_prog,
const ebpf_verifier_options_t& options) {
vector<vector<string>> notes;
return unmarshal(raw_prog, notes, options);
}
Expand Down
13 changes: 7 additions & 6 deletions src/ir/unmarshal.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
// SPDX-License-Identifier: MIT
#pragma once

#include <expected>

@coderabbitai coderabbitai Bot Apr 15, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== C++ standard configuration =="
rg -n --iglob 'CMakeLists.txt' --iglob '*.cmake' 'CMAKE_CXX_STANDARD|CXX_STANDARD|cxx_std_'

echo
echo "== expected feature usage / compatibility shims =="
rg -n 'std::expected|<expected>|tl::expected|expected.hpp|std::unexpected'

Repository: vbpf/prevail

Length of output: 1136

🏁 Script executed:

cd examples/using_installed_package && cat -n CMakeLists.txt

Repository: vbpf/prevail

Length of output: 727

🏁 Script executed:

grep -r "unmarshal\|ir" examples/using_installed_package/ --include="*.cpp" --include="*.hpp" --include="*.h"

Repository: vbpf/prevail

Length of output: 173

🏁 Script executed:

cat -n examples/using_installed_package/main.cpp

Repository: vbpf/prevail

Length of output: 1221

🏁 Script executed:

cat -n src/prevail.hpp | head -50

Repository: vbpf/prevail

Length of output: 103

🏁 Script executed:

grep -n "unmarshal" src/prevail.hpp

Repository: vbpf/prevail

Length of output: 104

🏁 Script executed:

find . -name "prevail.hpp" -type f

Repository: vbpf/prevail

Length of output: 38

🏁 Script executed:

git ls-files | grep -E "\.hpp$" | head -20

Repository: vbpf/prevail

Length of output: 535

🏁 Script executed:

git ls-files | grep -i "prevail" | grep -E "\.(hpp|h)$"

Repository: vbpf/prevail

Length of output: 38

🏁 Script executed:

rg -n "unmarshal" --iglob "*.hpp" --iglob "*.h" src/

Repository: vbpf/prevail

Length of output: 977

🏁 Script executed:

cat -n src/ebpf_verifier.hpp

Repository: vbpf/prevail

Length of output: 372

🏁 Script executed:

grep -rn "public\|install" CMakeLists.txt | head -30

Repository: vbpf/prevail

Length of output: 577

🏁 Script executed:

cat -n CMakeLists.txt | sed -n '240,265p'

Repository: vbpf/prevail

Length of output: 1084

🏁 Script executed:

cat -n src/ebpf_verifier.hpp

Repository: vbpf/prevail

Length of output: 372

C++23 features in public headers break C++20 consumer builds.

The project is configured for C++23 (CMakeLists.txt:33), but unmarshal.hpp exposes std::expected (C++23 only) in the public API via ebpf_verifier.hpp. This prevents C++20-configured consumers like examples/using_installed_package from using the installed library headers. Either restrict C++23 features to implementation-only files, provide a C++20-compatible compatibility layer, or document this as a breaking requirement for library consumers.

🧰 Tools
🪛 Clang (14.0.6)

[error] 5-5: 'expected' file not found

(clang-diagnostic-error)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/ir/unmarshal.hpp` at line 5, unmarshal.hpp currently exposes the C++23
header <expected> to public APIs (via ebpf_verifier.hpp); remove the direct
public dependency by introducing a small compatibility alias (e.g.,
expected_compat.hpp) that conditionally uses std::expected when __cplusplus >=
202302L and falls back to a header-only tl::expected (or a project-local
Expected<T,E>) for C++20, then replace `#include` <expected> in unmarshal.hpp with
`#include` "expected_compat.hpp" and update any public function signatures that
reference std::expected to use the compatibility alias (e.g., Expected<T,E>) so
C++20 consumers can include the installed headers.

@elazarg elazarg Apr 15, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Alan-Jowett do we need unmarshal.hpp as part of the API?

@Alan-Jowett Alan-Jowett Apr 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we currently call prevail::unmarshal() directly in 4 places across verifier_service.cpp and Verifier.cpp, and ebpf_verifier.hpp includes unmarshal.hpp. However, in all cases we immediately pass the InstructionSeq to the next step (Program::from_sequence, print, or analyze) — we never use InstructionSeq on its own.

So if prevail provided higher-level APIs that accept RawProgram directly (e.g., Program::from_raw(RawProgram, notes, options) that internalizes the unmarshal step, and a disassemble-to-stream function), we could stop depending on unmarshal.hpp and InstructionSeq entirely. That would also resolve the C++23 public API concern since std::expected would stay internal.

Our project currently builds with C++20, so exposing std::expected in public headers would require us to bump our standard.

@coderabbitai coderabbitai Bot Apr 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

@elazarg elazarg Apr 15, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this?
https://github.com/vbpf/prevail/pull/1074/changes#diff-6714101bca0d9472baf09f9c8eaf5708d12f6fa8f5dc3160e52f319eb2e32310

/// Build a Program directly from a raw (ELF-bytecode) representation.
/// On failure returns std::nullopt and appends the error message to `notes`
/// (the same vector that collects unmarshal warnings).
static std::optional<Program> from_raw(const RawProgram& raw_prog, std::vector<std::vector<std::string>>& notes,
                                       const ebpf_verifier_options_t& options);
/// Write a textual disassembly of `raw_prog` to `out`. On failure writes the
/// error message to `out` and returns false.
bool disassemble(const RawProgram& raw_prog, const ebpf_verifier_options_t& options, std::ostream& out,
                 const std::optional<Label>& label_to_print = {}, bool print_line_info = false);

#include <string>
#include <variant>
#include <vector>

#include "ir/syntax.hpp"
Expand All @@ -17,13 +17,14 @@ namespace prevail {
*
* \param raw_prog is the input program to parse.
* \param[out] notes is a vector for storing errors and warnings.
* \param options unmarshalling options
* \return a sequence of instructions if successful, an error string otherwise.
*/
std::variant<InstructionSeq, std::string> unmarshal(const RawProgram& raw_prog,
std::vector<std::vector<std::string>>& notes,
const prevail::ebpf_verifier_options_t& options);
std::variant<InstructionSeq, std::string> unmarshal(const RawProgram& raw_prog,
const prevail::ebpf_verifier_options_t& options);
std::expected<InstructionSeq, std::string> unmarshal(const RawProgram& raw_prog,
std::vector<std::vector<std::string>>& notes,
const ebpf_verifier_options_t& options);
std::expected<InstructionSeq, std::string> unmarshal(const RawProgram& raw_prog,
const ebpf_verifier_options_t& options);

Call make_call(int imm, const ebpf_platform_t& platform);
} // namespace prevail
11 changes: 5 additions & 6 deletions src/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,16 +201,15 @@ int main(int argc, char** argv) {
const RawProgram& raw_prog = raw_progs.back();

// Convert the raw program section to a set of instructions.
std::variant<InstructionSeq, std::string> prog_or_error = unmarshal(raw_prog, ebpf_verifier_options);
if (auto prog = std::get_if<string>(&prog_or_error)) {
std::cout << "unmarshaling error at " << *prog << "\n";
const auto inst_seq = unmarshal(raw_prog, ebpf_verifier_options);
if (!inst_seq.has_value()) {
std::cout << "unmarshaling error at " << inst_seq.error() << "\n";
return 1;
}

auto& inst_seq = std::get<InstructionSeq>(prog_or_error);
if (!asmfile.empty()) {
std::ofstream out{asmfile};
print(inst_seq, out, {});
print(*inst_seq, out, {});
print_map_descriptors(thread_local_program_info->map_descriptors, out);
}

Expand All @@ -226,7 +225,7 @@ int main(int argc, char** argv) {
}
}
const auto verbosity = ebpf_verifier_options.verbosity_opts;
const Program prog = Program::from_sequence(inst_seq, raw_prog.info, ebpf_verifier_options);
const Program prog = Program::from_sequence(*inst_seq, raw_prog.info, ebpf_verifier_options);

if (!dotfile.empty()) {
print_dot(prog, dotfile);
Expand Down
14 changes: 6 additions & 8 deletions src/test/ebpf_yaml.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,24 +558,22 @@ ConformanceTestResult run_conformance_test_case(const std::vector<uint8_t>& memo
raw_prog.info.platform = &platform;

// Convert the raw program section to a set of instructions.
std::variant<InstructionSeq, std::string> prog_or_error = unmarshal(raw_prog, {});
if (auto prog = std::get_if<std::string>(&prog_or_error)) {
std::cerr << "unmarshaling error at " << *prog << "\n";
return ConformanceTestResult{.success = false, .r0_value = Interval::top(), .error_reason = *prog};
const auto inst_seq = unmarshal(raw_prog, {});
if (!inst_seq.has_value()) {
std::cerr << "unmarshaling error at " << inst_seq.error() << "\n";
return ConformanceTestResult{.success = false, .r0_value = Interval::top(), .error_reason = inst_seq.error()};
}

const InstructionSeq& inst_seq = std::get<InstructionSeq>(prog_or_error);

ebpf_verifier_options_t options{};
if (debug) {
print(inst_seq, std::cout, {});
print(*inst_seq, std::cout, {});
options.verbosity_opts.print_failures = true;
options.verbosity_opts.print_invariants = true;
options.verbosity_opts.simplify = false;
}

try {
const Program prog = Program::from_sequence(inst_seq, info, options);
const Program prog = Program::from_sequence(*inst_seq, info, options);
const AnalysisResult result = analyze(prog, pre_invariant);
return ConformanceTestResult{.success = !result.failed, .r0_value = result.exit_value};
} catch (const std::exception& ex) {
Expand Down
4 changes: 1 addition & 3 deletions src/test/test_elf_loader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,9 +559,7 @@ TEST_CASE("rewrite_extern_constant_load bails out on values exceeding int32 rang
REQUIRE(rewrite_extern_constant_load(insts, 0, 0xFFFFFFFFFFFFFFFFULL));
}

SECTION("0x80000000 exceeds int32 — bails out without mutation") {
check_bailout_preserves_program(0x80000000ULL);
}
SECTION("0x80000000 exceeds int32 — bails out without mutation") { check_bailout_preserves_program(0x80000000ULL); }

SECTION("0x100000000 exceeds int32 — bails out without mutation") {
check_bailout_preserves_program(0x100000000ULL);
Expand Down
20 changes: 8 additions & 12 deletions src/test/test_failure_slice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,8 @@ static std::vector<FailureSlice> get_failure_slices(const std::string& filename,
REQUIRE(raw_progs.size() == 1);

const RawProgram& raw_prog = raw_progs.back();
auto prog_or_error = unmarshal(raw_prog, options);
auto inst_seq = std::get_if<InstructionSeq>(&prog_or_error);
REQUIRE(inst_seq != nullptr);
auto inst_seq = unmarshal(raw_prog, options);
REQUIRE(inst_seq.has_value());

const Program prog = Program::from_sequence(*inst_seq, raw_prog.info, options);
auto result = analyze(prog);
Expand Down Expand Up @@ -183,9 +182,8 @@ TEST_CASE("print_failure_slices produces structured output", "[failure_slice][pr
REQUIRE(raw_progs.size() == 1);

const RawProgram& raw_prog = raw_progs.back();
auto prog_or_error = unmarshal(raw_prog, options);
auto inst_seq = std::get_if<InstructionSeq>(&prog_or_error);
REQUIRE(inst_seq != nullptr);
auto inst_seq = unmarshal(raw_prog, options);
REQUIRE(inst_seq.has_value());

const Program prog = Program::from_sequence(*inst_seq, raw_prog.info, options);
auto result = analyze(prog);
Expand Down Expand Up @@ -218,9 +216,8 @@ TEST_CASE("passing program produces no failure slices", "[failure_slice][integra
REQUIRE(raw_progs.size() == 1);

const RawProgram& raw_prog = raw_progs.back();
auto prog_or_error = unmarshal(raw_prog, options);
auto inst_seq = std::get_if<InstructionSeq>(&prog_or_error);
REQUIRE(inst_seq != nullptr);
auto inst_seq = unmarshal(raw_prog, options);
REQUIRE(inst_seq.has_value());

const Program prog = Program::from_sequence(*inst_seq, raw_prog.info, options);
auto result = analyze(prog);
Expand Down Expand Up @@ -252,9 +249,8 @@ TEST_CASE("assume guard registers become relevant in slice", "[failure_slice][in
ElfObject elf{sample, options, &g_ebpf_platform_linux};
const auto& raw_progs = elf.get_programs("xdp");
REQUIRE(raw_progs.size() == 1);
auto prog_or_error = unmarshal(raw_progs.back(), options);
auto inst_seq = std::get_if<InstructionSeq>(&prog_or_error);
REQUIRE(inst_seq != nullptr);
auto inst_seq = unmarshal(raw_progs.back(), options);
REQUIRE(inst_seq.has_value());
const Program prog = Program::from_sequence(*inst_seq, raw_progs.back().info, options);

bool found_assume_in_slice = false;
Expand Down
Loading
Loading