feat(variables): add initial version of variables.c test fixture - #1038
feat(variables): add initial version of variables.c test fixture#1038JoshuaMoelans wants to merge 13 commits into
variables.c test fixture#1038Conversation
- build.sh to generate binaryu (with O0 to avoid optimizing out everything) - .snap for validating - updated test_objects with new test_elf_variables - smol README with instructions how to run/rebuild/refresh the snapshot/add coverage/uncovered items Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
JoshuaMoelans
changed the title
variables.c test fixture
Fold VariablesDebug's duplicate tree walk into FunctionsDebug behind DebugOptions, and reduce VariablesDebug to a Debug impl for a single function's variables, replacing write_variables. Snapshots unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Pin the Docker image to gcc:14.4.0 (the version the committed fixture was built with) instead of the floating gcc:14 tag. - Drop the false claim that ./build.sh outside Docker gives the same result: DW_AT_comp_dir embeds the build directory. - Fix the garbled rebuild instructions in the README. - Fix the primitives() comment: the enum is PrimitiveTypeEncoding, and Address is deliberately not covered. - Rebuild the fixture with the pinned image (comment edit shifted line numbers in the debug info; snapshot unchanged). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
In relative mode, saturating_sub silently clamped a location address below the function base to 0x0, which prints as a plausible-looking whole-function range. A backend emitting wrongly-based addresses (e.g. module-relative vs. absolute) would produce an accepted snapshot instead of a failure. checked_sub().expect() makes the violation panic the test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Only two of the eight nominal flag combinations were ever constructed, and they were fully correlated: variables was always true and lines was always !relative. A two-variant enum (Full / RelativeVariables) encodes only the states that exist, deletes the dead variables guard, and removes the 0-sentinel base field on VariablesDebug. No output changes; all snapshots unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cd to the script's own directory before compiling, following the precedent in fixtures/wasm/emscripten/build.sh (the POSIX-sh form; the arm64 script's BASH_SOURCE variant is a bashism). Inside the documented Docker invocation this is a no-op: the working directory is already the script directory, so DW_AT_comp_dir and the fixture binary are unchanged (verified byte-identical). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
JoshuaMoelans
changed the title
variables.c test fixturevariables.c test fixture
| /// Only the variables of each function, with location ranges relative to the start of their | ||
| /// function, so a snapshot only changes when variable debug info changes, not when the | ||
| /// fixture is merely relinked or relocated. Toolchain changes might still churn the snapshot. | ||
| RelativeVariables, |
There was a problem hiding this comment.
What problem is this trying to solve? Why does a toolchain change not impact the snapshot, variables may now be stored in different locations.
There was a problem hiding this comment.
@Dav1dde do you mean in general, using the RelativeVariables over the Full DebugMode? Or just the phrasing of the comment?
The last sentence is meant to indicate that toolchain changes do indeed churn the snapshot, but if it doesn't read cleanly I can reword it for sure.
There was a problem hiding this comment.
IIUC the RelativeVariables that I (+ clanker) introduced was just meant to simplify the variable extraction snapshot, but after talking with @szokeasaurusrex about it too, it makes sense to just reuse the existing debug format since we never rewrite the snapshot by hand (and just regenerate it). This simplified the PR quite a bit 008b8d7
|
|
||
| To keep remote up-to-date, commit the rebuilt binary together with the updated snapshot. | ||
|
|
||
| ## Adding coverage |
There was a problem hiding this comment.
I am not sure what this is supposed to tell me other than "prefer to add a new function over changing existing ones". Where this is a fine suggestions, though I don't agree with the reasoning, there is nothing wrong with the fixture changing.
There was a problem hiding this comment.
Do you feel like we should just cut out the reasoning in the paragraph (I think keeping the first sentence as a suggestion by itself is probably enough). It wasn't meant as a "try never to change the existing parts of the fixture", more of a "if you want to keep changes to the fixture minimal, adding functions is self-contained" hint.
Store the variables fixture build script and source next to the binary in fixtures/linux/variables/, matching the other fixtures, and trim the fixture docs to the essentials. The binary is rebuilt because the embedded DW_AT_comp_dir follows the new build location. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- we added RelativeVariables to reduce snapshot churn, but tbqh this is unnecessary bc we can regenerate the snapshot anyway. Keep using the normal debug mode like for the other tests
loewenheim
left a comment
loewenheim
left a comment
There was a problem hiding this comment.
LGTM, assuming the snapshot contains what you expect it to.
3 participants
test_objects__elf_variables.snapfor validatingtest_objectswith newtest_elf_variablesresolves https://linear.app/getsentry/issue/INGEST-1087/dwarf-add-new-test-fixture-for-variables
To be followed up with #1042 (or maybe we merge that one in here, not sure what flows more naturally?)