cgen: use .incbin assembler directives for $embed_file, eliminate the 5MB limit#27438
cgen: use .incbin assembler directives for $embed_file, eliminate the 5MB limit#27438MagicFun1241 wants to merge 6 commits into
Conversation
…he 5MB limit and 6x C-source expansion Replace C hex array literals (0x2f,0x2a,...) with GNU assembler .incbin directives that write raw bytes directly into .rodata/.S files, similar to Go's //go:embed SRODATA approach. For GCC/Clang/TCC targets, each embedded file is written as a .S assembly file with .incbin pointing at the original binary. The .S file uses #if defined(__APPLE__) to emit the correct Mach-O (.section __TEXT,__const) or ELF (.section .rodata) directives, and handles macOS's extra _ C symbol prefix. Compressed embeds keep their temp .bin files alive for .incbin to reference at assembly time. The C hex array path remains as fallback for MSVC, Windows cross-compilation, -o .c output, and .o output mode, ensuring zero regressions. Embedded file .S symbols use fnv1a hash-based naming (_v_embed_blob_<hash>) to avoid collisions during parallel test execution. The gen_c() return type stays strings.Builder — embed data flows through Builder struct fields (embedded_asm, embedded_o_files, embedded_temp_files) and is compiled to .o files in gen_c() (parallel_cc) or build_c() (serial), then added to linker args via the same path as thirdparty .o files. Benchmark (macOS, Apple clang 17): - 10MB embed: 0.5s vs 7.6s compile (15x faster) - 10MB embed: 107MB vs 1143MB peak compiler RSS (10x less) - 10MB embed: ~0 vs 53.5MB C source (eliminated) - 25MB embed: 0.5s compile (was impossible with C hex) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c27d1b04d7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Wasm uses a different object format (WASM, not ELF/Mach-O). Our .S files contain ELF/Mach-O section directives that would either fail to assemble or produce host-arch .o files that can'\''t link into the wasm output. The C hex array fallback is correct for these targets. Also adds a safety check in compile_embedded_asm_files() that verror()s if wasm data somehow reaches the .S assembly path. Found by Codex review on PR vlang#27438. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b56b0e172
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…sembly Three fixes from Codex review (PR vlang#27438): 1. wasm32 targets: .S files with ELF/Mach-O directives produce host-arch .o files incompatible with wasm. Already disabled .emcc from allow-list in prior commit; now also adds a verror() safety check in compile_embedded_asm_files(). 2. build_module (-usecache): V compiles each module as a standalone .o. Embed .o files are separate and cannot be merged into the cached module .o, causing unresolved _v_embed_blob_* symbols at final link time. Disable incbin for build_mode == .build_module, with verror() safety check. 3. Windows/COFF: .type and .size are ELF-specific directives rejected by COFF assemblers (clang -target x86_64-w64-windows-gnu). Add #elif defined(_WIN32) branch using .section .rdata without .type/.size. The .size guard also excludes _WIN32. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review now |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a5e85bb6f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…\` from assembler candidates Two fixes from Codex review (PR vlang#27438): 1. Linux cross-compile arch mismatch: the previous condition only checked host_os != .linux, missing the case where both are Linux but target arch differs (e.g. x86_64 host → arm64 target). Without -target, the assembler produces a host-arch .o that fails to link. Now also checks host_arch vs pref.arch, adding -target whenever they differ. 2. Raw \`as\` removed from assembler candidate lists: GNU \`as\` doesn't preprocess #if directives in .S files and has no -c flag, so it can't assemble our generated .S files. Removed from both find_system_assembler() and should_use_incbin_embed() TCC fallback. Only clang/gcc/cc are valid candidates (they preprocess and accept -c). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e799c94164
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… equality Two embeds of the same compressed file each generate a random ULID for compressed_temp_path, so struct equality (!in) fails to deduplicate them, producing duplicate case labels and index entries in generated C. Fix: deduplicate by EmbeddedFile.hash() which excludes compressed_temp_path, so the first embed keeps its temp path and subsequent duplicates are skipped. Found by Codex review on PR vlang#27438. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
|
Instead of writing a |
|
Yes. Might be an option |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f83697bc1b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… parallel-build races When two V processes embed the same file concurrently (parallel v test), both would write to the same _v_embed_blob_<hash>.S/.o in the shared vtmp_dir, causing race conditions. Add a per-build random ULID suffix to the filenames so each build writes to unique paths. Symbol names remain deterministic (hash-based) — only the filenames are randomized. The Gen struct gains an embed_build_id field initialized with rand.ulid() and propagated through GenOutput. Found by Codex review on PR vlang#27438. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@JalonSolov I can try to create different PR with direct .o writing approach but I think there will be near 200-300 LOC per format. Is it acceptable? |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57ef01b2a0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Note: embedded .o files are NOT added to ofiles here. They are already | ||
| // included via ccoptions.o_args (set up in cc() during the parallel_cc | ||
| // str_args generation), which get_linker_args() picks up. |
There was a problem hiding this comment.
When -parallel-cc is used with $embed_file on an incbin-enabled target, the generated C references _v_embed_blob_*, but the compiled embed .o files are never added to the final alink command. I checked get_linker_args() in vlib/v/builder/cc.v:1152-1165; it only returns linker flags/ldflags, not ccoptions.o_args, so the assumption in this comment is false and the link step will fail with unresolved embed symbols. Add b.embedded_o_files to ofiles or directly to alink before linking.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex .o files reach the parallel-cc link command via ccoptions.o_args, which is part of compile_args.
Link command seems to use scompile_args_for_linker, which includes o_args.
Verified with -parallel-cc -cc cc:
'cc' ... '/tmp/v_501/_v_embed_blob_10850619718018418698_01KTYRY5K11Y2HV27FFFRFRTE1.o' ... -o '.../par_bin' '.../out_0.o' ... '.../out_x.o' -lgc -ldl -lpthread
Embed .o appears in both the per-file cc_cmd and the final link_cmd. The comment is correct
There was a problem hiding this comment.
To use Codex here, create a Codex account and connect to github.
|
I don't think 200-300 loc per format (there aren't that many.. ELF, PE, etc.) in the compiler will be that noticeable, as long as it makes this process simpler/easier. Besides, a large amount of the code should be sharable between the formats... the handling of the bytes in the file being embedded. The main thing that will be different will be the name of the output ( |
2 participants
Summary
Replace C hex array literals with GNU assembler
.incbindirectives for$embed_file, matching Go's//go:embedSRODATA approach. Raw bytes are written directly into.rodatavia assembly, eliminating the intermediate C source text expansion.Problem
V's current embed implementation generates C hex array literals for embedded file data. A 10MB file produces ~53MB of C source, causing:
Approach
For GCC, Clang, MinGW, and TCC (when a system assembler is available), each embedded file is written as a
.Sassembly file using.incbinpointing at the original binary. The.Sfile uses#if defined(__APPLE__)/#elif defined(_WIN32)/#elseto emit the correct section directives for each platform (Mach-O__TEXT,__const, COFF.rdata, ELF.rodata) and handle macOS's extra_C symbol prefix. ELF-only directives (.type,.size) are excluded on Windows.The C hex array path remains as fallback for MSVC, Windows cross-compilation,
-o .coutput,.ooutput mode, wasm targets, andbuild_module(-usecache) mode — zero regressions.Compressed embeds (
.zlib) keep their temp.binfiles alive for.incbinto reference at assembly time, with cleanup scheduled viacleanup_files.Symbol names use fnv1a hash-based naming (
_v_embed_blob_<hash>) to avoid collisions during parallel test execution. Deduplication usesEmbeddedFile.hash()instead of struct equality to handle compressed files with random temp paths.gen_c()return type staysstrings.Builder— embed data flows throughBuilderstruct fields (embedded_asm,embedded_o_files,embedded_temp_files), compiled to.oingen_c()(parallel_cc) orbuild_c()(serial), then added to linker args via the same path as thirdparty.ofiles.Design decisions
Why .incbin instead of alternatives considered
.incbin(chosen)freadat runtime$embed_fileobjcopy --input binaryPlatform fallbacks
.incbin.Sfiles.incbin.Sfilesasexcluded: no-cflag, no#ifpreprocessing.incbin.ofiles.Sbuild_module(-usecache).ocan't be merged into cached module.o-o .c/-o .o/ stdout.ointoWindows / COFF handling
The
.Sassembly uses#elif defined(_WIN32)to emit.section .rdata(COFF read-only data) instead of.section .rodata(ELF), and skips.type/.sizedirectives that COFF assemblers reject.TCC handling
Two-level fallback:
should_use_incbin_embed): check if clang/gcc/cc is available. If yes →.incbin. If no → C hex arrays.compile_embedded_asm_files): if TCC reaches this point, find the system assembler. If not found →verror()with clear message.Linux cross-architecture
When cross-compiling Linux x86_64 → Linux arm64 (same OS, different arch), the
.Sfile must be assembled with-targetmatching the cross-compilation triple. The condition now checkshost_os != .linux || host_arch != b.pref.arch.Deduplication
EmbeddedFilededuplication useshash()(based on apath, compression_type, is_compressed, len) instead of struct equality, becausecompressed_temp_pathcontains a random ULID that would prevent deduplication of the same compressed file embedded twice.Hash-based symbol naming
Parallel test execution (
v test) compiles multiple binaries simultaneously. Sequential names like_v_embed_blob_0collide; hash-based names (_v_embed_blob_<fnv1a_hash>) ensure uniqueness per file path.Parallel cc ordering
parallel_cc()runs insidegen_c(), beforebuild_c(). Embed.ofiles are compiled in-place ingen_c()with a guard (embedded_o_files.len == 0) to prevent double-compilation inbuild_c().Performance comparison
macOS, Apple clang 17, averaged over 3 runs: