We're explicitly specifying bash in enough rules that adopting it at the
top-level is probably best for minimizing surprises.
Really the only bash feature we need is process substitution
(`a <(b) <(c)`), but it's a hard feature to pass on.
Though it will be interesting to see if any users run into problems with
this.
So, avg seems like a poor way to merge throughput results, at least for
bench_rbyd (avg create/delete throughputs were wildly different).
To fix this, changed make benchmarks and friends to a two step
calculation:
1. find max results: -fn='max(n)' -ft='float(bench_simtime)/1.0e9'
2. calculate throughput: -fthroughput='avg(float(n) / max(t, 1.0e-9))'
As a plus, this is probably more robust toward accidentally introducing
new by fields.
Note this does require two csv.py calls. csv.py doesn't support exprs
after folding as an intentional simplification (in theory it's always
possible to chain multiple csv.py calls together). In make this can be a
bit annoying since we need SHELL=/bin/bash for subprocess substitution
(for benchmarks-diff specifically), but that's not the end of the world.
--
Also changed -Si='min(enumerate())' -> -Si -Fi='min(enumerate())', in
case users override SUMMARYFLAGS.
This is mostly useful for finding slow test cases, but may be useful for
benches in the future?
I've been running a separate script for this, but adding a rule to the
Makefile makes it a bit easier:
TESTMARKS=1 make test -j && make testmarks-bottlenecks
- Sort by -Si='min(enumerate())'
This was an unexpectedly neat trick for ordering result based on input
csv, without disabling folding like -i/-I.
Note the min is needed because the field is summed before sorting, so
an early case with many permutations can end up after later cases by
default. Because -i/-I also disables folding, it doesn't have this
problem.
- Adopted m -> probe rename.
- Adopted delta expr, so we can now show n correctly, without
accidentally summing already cumulative results.
- Changed benchmark fields simtime+simthroughput -> n+t+throughput
- Adopted throughput=avg(throughput), so TOTAL is somewhat useful?
Unsure if this is the correct way to merge throughput results.
We already hinted PERFGEN in make perf's help text. This extends the
idea to the other rules that depend on env variables.
The default behavior of erroring because make test/bench was run without
the right env variable is confusing enough.
The main changes:
- Added TEST_DEFINES and BENCH_DEFINES to allow overriding the default
test/bench define header:
-DTEST_DEFINES=my_test_defines.h
Note these are VERY different from LFS_DEFINES upstream. They aren't a
typical header file, and are included multiple times with various
query macros.
It's hacky, but works surprisingly well.
Or maybe I'll just do anything to avoid having to write multiline
macros. Ugh, backslashes.
- Moved more logic into bench/test_defines.h, including everything
needed to integrate other filesystems out-of-tree.
This mostly meant moving all of the cfg initialization logic into its
own query macro (replacing the BENCH/TEST_CFG field macros).
But this also includes a bunch of small tweaks encountered while trying
to get external benchmarks running again.
The external benchmarks include several other filesystems (littlefs2,
SPIFFS, Yaffs2), and I'm hoping this injectable/queryable header thing
will do a good job at avoiding a maintenance headache. (At least a
better job than forking bench_runner.c, which was the previous
solution.)
These have been battle-tested in external benchmarks, and have proven
useful for finding a runtime estimate on stack+heap usage.
Of course, to be realistic they need to be cross-compiled and run under
QEMU (which does work!), but even on x86_64 they provide a nice insight
into RAM usage. In practice the only real difference is pointer width
anyways.
---
Enabled by default for the bench runner, these are available if
TEST/BENCH_YES_HEAP and/or TEST/BENCH_YES_STACK are defined.
(This default is provided by the Makefile. At least heap measurements
rely on linker flags, so it probably doesn't make sense to default
enable in the bench runner itself.)
Stack vs heap rely on slightly different mechanisms:
- Stack: Uses GCC's __builtin_frame_address(0) to measure the current
stack usage on entry to every bd operation.
- Heap: Relies on GCC's -Wl,--wrap flags to intercept every malloc/free
call, to track the current heap usage.
These are available via bench/test macros:
- BENCH_STACK() - Maximum stack usage of the current run
- BENCH_STACK_CURRENT() - Current stack usage
- BENCH_HEAP() - Maximum heap usage of the current run
- BENCH_HEAP_CURRENT() - Current heap usage
Note BENCH_STACK_CURRENT() can be useful for separating out the bench's
ctx from total stack usage, similarly to our static analysis.
---
One surprising outcome is that these heap hooks trivially implement a
memory leak detector. Maybe that could be useful in the test_runner as a
cheaper alternative to Valgrind?
The value of simtime isn't actually the simtime value, but the
simulated throughput, which is easy enough for our csv.py script to
calculate (with a daintily placed max to avoid divide-by-zero).
Throughput has the benefit of being somewhat size-agnostic, making
cross-benchmark comparisons easier.
I guess it's technically possible to do something similar with
readed/progged/erased numbers, but conceptually that would be really
confusing...
---
Also renamed test/bench_time -> test/bench_runtime to hopefully prevent
confusion between the two time spaces.
Tweaked: 1400x750 -> 1125x525 (1.5x codemapsvg.py's default)
This is now derived (1.5x) from the default dimensions in codemapsvg.py.
This matches the dimensions that ended up used for the preliminary v3
benchmarks, which are a bit more convenient on devices with smaller
screens.
As for where the 750x350 resolution came from, I'm not entirely sure.
Maybe a random Matplotlib example? It approximates a 2:1 aspect ratio
but with 25 pixels carved out for margins.
Note we like wide aspect ratios over pretty aspect ratios like 16:9,
golden ratio, etc, here:
1. We often cram things into the margins (legends, stack usage, etc)
2. English text is much wider than it is tall (this commit message has
an aspect ration of ~3:1), so wider aspect ratios help readability
Jumping to prototypes in header files is extremely useful, because
that's usually where all the documentation is. But jumping to prototypes
in C files is a bit much. These are usually just uncomment definitions
to keep the compiler happy, and make navigation a bit of a pain.
Unfortunately it doesn't seem like ctags supports per-file-type tag
kinds (at least I couldn't find it in the documentation), but running
ctags twice with the --append flag seems to work.
This was missing from the big lfs -> lfs3 rename, probably because it
didn't actually break testing. It just prevents prettyasserts from
making LFS3_ASSERT pretty.
There's already been a bunch of benchmarking targeting the current hash,
and we're probably going to find other missed prefixes in corners of the
codebase anyways, so I'm not going to bother rebasing.
- codemapd3.py -> codemapsvg.py
- dbgbmapd3.py -> dbgbmapsvg.py
- treemapd3.py -> treemapsvg.py
Originally these were named this way to match plotmpl.py, but these
names were misleading. These scripts don't actually use the d3 library,
they're just piles of Python, SVG, and Javascript, modelled after the
excellent d3 treemap examples.
Keeping the *d3.py names around also felt a bit unfair to brendangregg's
flamegraph SVGs, which were the inspiration for the interactive
component. With d3 you would normally expect a rich HTML page, which is
how you even include the d3 library.
plotmpl.py is also an outlier in that it supports both .svg and .png
output. So having a different naming convention in this case makes
sense to me.
So, renaming *d3.py -> *svg.py. The inspiration from d3 is still
mentioned in the top-level comments in the relevant files.
So:
$(filter-out %.t.c %.b.c %.a.c,$(wildcard bd/*.c))
Instead of:
$(filter-out $(wildcard bd/*.t.* bd/*.b.*),$(wildcard bd/*.c))
The main benefit is we no longer need to explicitly specify all
subdirectories, though the single wildcard is a bit less flexible if
test.py/bench.py ever end up with other non-C artifacts.
Unfortunately only a single wildcard is supported in filter-out.
Mainly formatting/comment things, but also a couple tweaks:
- Changed BUILDDIR mkdir hack to infer directories from SRC, TESTS,
TEST_SRC, etc
Avoids a hardcoded list of build directories.
- Added $(BUILDDIR)/%.c -> $(BUILDDIR)/%.{o,ci,s} rules
Without these, make doesn't know how to build .o files that depend on
generated .c files (.t.c, .b.c, .a.c, etc) when using an external
BUILDDIR.
Ok so, funny story, looks like we won't actually need pure-tree
bshrubs/btrees.
It _is_ true that the single-parent constraint imposed by pure-trees can
enable a wider range of algorithms. But looking forward into the planned
design, we just happen to not need this constraint at all. I made a
mistake here:
1. Block allocation - On paper block allocation benefits the most from
the single-parent constraint. But we have another daggish problem,
how do we efficiently account for in-flight/open btrees?
Naively, you might think we can just traverse all open btrees during
allocation, since we shouldn't have _that_ many. But this scales
O(n^2) when writing a large file. The key observation being that open
files reference on-disk btrees and are _not_ RAM constrained.
The current solution involves tree-diffing in order to figure out
bmap updates. Which, humorously, works perfectly fine even if the
trees are dags.
2. Error correction - I just completely forgot that the current plans
for block redundancy require the ddtree.
Each block gets mapped into the dense ddtree, with subranges of the
ddtree grouped into parity groups backed by the ptree. Instead of
bptrs, file btrees store indirect ddkeys into the ddtree. No bptrs?
No dag problem!
This is still a problem if we ever support naive data redund (redund
blocks in a bptrs), but that's out of scope for other reasons
(basically just a lot more code).
So reverting. Allowing dags allows for much faster random writes, at
least in theory.
---
For now I'm still keeping the dag-avoidance in lfsr_file_flush_ around
under the LFS_NONDAG ifdef. This will likely be dropped at some point,
but I'm curious how it affects benchmarks.
Ugh, and of course the unused label makes GCC unhappy. Added
-Wno-unused-label to CFLAGS because labels have other uses besides just
being goto targets (debug targets, code organization, etc).
We probably use labels more that other libraries because to littlefs's
no-recursion requirement.
Code changes minimal, still not sure where that stack difference comes
from:
code stack ctx
before: 35740 2424 640
after: 35736 (-0.0%) 2440 (+0.7%) 640 (+0.0%)
So instead of:
CFLAGS='-DLFS_YES_REVDBG=1' make
You can just do:
LFS_YES_REVDBG=1 make
I've been hesitant to add this, as I've never seen this pattern in
another project (why?), but it's just too convenient to not give it a
try.
Instead, make codemap/codemap-tiny just generate the relevant .svgs:
- dropped make codemap
- dropped make stackmap
- dropped make ctxmap
- make codemap-svg -> make codemap
- make codemap-tiny-svg -> make codemap-tiny
The ascii-art codemaps just really aren't useful due to their low
resolution. We might as well repurpose the relevant make rules to save
keystrokes.
Though I did keep the ascii-art as a step in make codemap/codemap-tiny,
just for fun.
Mainly adopting the added flexibility in csv.py, also adding make
codemap-svg and friends for code map generation:
- Split result commands into separate result, result-csv, and
result-diff commands so csv generation is explicit.
So make result no longer implicitly overwrites csv files:
make code
make code-csv -.
make code |
make code-diff <'
This gives more control over result diffing.
make code-csv _is_ more or less just a dependency on the lfs.code.csv
rule, but it avoids BUILDDIR mess and is easier to remember.
- Added make codemap/stackmap/ctxmap for in-terminal code/stack/ctx
ascii art.
I was a bit on the fence on these, since the result is more pretty
than useful, but eh, can always drop them in the future.
- Added make codemap-svg/codemap-tiny-svg for generating interactive
codemap svgs.
This raised an interesting question if the make commands should
generate light or dark mode svgs. I settled on dark mode since that's
what I personally find the most useful.
I think the way this will breakdown is with dark mode generally used
for development, and light mode generally used for published material.
And it's not too hard to run the script outside of the Makefile for
publishing. Or override CODEMAPFLAGS.
- Adopted implicit prefixing, -q, etc. This simplifies some of the more
complicated csv.py invocations (make summary, make funcs, etc).
See make help for a full list of commands.
In addition to providing more functionality for creating -b/--by fields,
this lets us remove strings from the expr parser. Strings had no
well-defined operations and could best be described as an "ugly wart".
Maybe we'll reintroduce string exprs in the future, but for now csv.py's
-f/--field fields will be limited to numeric values.
As an extra plus, no more excessive quoting when injecting new -b/--by
fields.
---
This also fixed sorting on non-field fields, which was apparently
broken. Or at least mostly useless since it was defaulting to string
sorting.
Looks like this was never updated after changing the -Y/--summary +
-c/--compare hack to its own -Q/--small-table flag. Fortunately a single
character fix.
Unrelated, but I was considering dropping the make *-diff rules, until
the different compile-time targets proved they are _very_ useful when
jumping around various commits/builds.
This reverts per-result source file mapping, and tears out of a bunch of
messy dwarf parsing code. Results from the same .o file are now mapped
to the same source file.
This was just way too much complexity for slightly better result->file
mapping, which risked losing results accidentally mapped to the wrong
file.
---
I was originally going to revert all the way back to relying strictly on
the .o name and --build-dir (490e1c4) (this is the simplest solution),
but after poking around in dwarf-info a bit, I realized we do have
access to the original source file in DW_TAG_compile_unit's
DW_AT_comp_dir + DW_AT_name.
This is much simpler/more robust than parsing objdump --dwarf=rawline,
and avoid needing --build-dir in a bunch of scripts.
---
This also reverts stack.py to rely only on the .ci files. These seem as
reliable as DW_TAG_compile_unit while simplifying things significantly.
Symbol mapping used to be a problem, but this was fixed by using the
symbol in the title field instead of the label field (which strips some
optimization suffixes?)
See previous commit for the issues with stack.py's current approach. I'm
convinced dwarf-info simply does not contain enough info to figure out
stack usage.
There is one last idea, which is to parse the dissassembly. In theory
you only need to understand calls, branches (for control-flow), and
push/pop instructions to figure out the worst-case stack usage. But this
would be ISA-specific and error-prone, so it probably shouldn't
_replace_ the -fcallgraph-info=su based stack.py.
So, out of ideas, reverting.
---
It's worth noting this isn't a trivial revert. There's a couple
interesting changes in stack.py:
- We now use .o files to map callgraph nodes to relevant symbol names.
This should be a bit more robust than relying only on the names in the
.ci files, and guarantees function names line up with other
symbol-based scripts (code.py, ctx.py, etc).
This also lets us warn on missing callgraph nodes, in case the
callgraph info is incomplete.
- Callgraph parsing should be quite a bit more robust now. Added a small
(and reusable?) Parser class.
- Moved cycle detection into result collection.
This should let us drop cycle detection from the table renderer
eventually.
There were a lot of small challenges (see previous commits), but this
commit reworks stack.py to rely only on dwarf-info and symbols to build
stack + callgraph info.
Not only does this remove an annoying dependency on a GCC-specific flag,
but it also should give us more correct stack measurements by only
penalizing calls for the stack usage at the call site. This should
better account for things like shrinkwrapping, which make the
-fcallgraph-info=su results look worse than they actually are.
To make this work required jumping through a couple hoops:
1. Map symbols -> dwarf entries by address (DW_AT_low_pc).
We use symbols here to make sure function names line up with other
scripts.
Note that there can be multiple dwarf entries with the same name due
to optimization passes. Apparently the optimized name is not included
because that would be too useful.
2. Find each functions' frame info.
This is stored in the .debug_frames section (objdump --dwarf=frames),
and requires _yet another state machine_ to parse, but gives us the
stack frame info for each function at the instruction level, so
that's nice.
3. Find call sites (DW_TAG_call_site).
The hierchical nesting of DW_TAG_lexical_blocks gets a bit annoying
here, but ultimately we can find all DW_TAG_call_sites by looking at
the DW_TAG_subprogram's children tags.
4. Map call sites to frame info.
This gets funky.
Finding the target function is simple enough, DW_AT_call_origin
contains its dwarf offset (but why is this the _origin_?). But we
don't actually know what address the call originated from.
Fortunately we do know the return address, DW_AT_call_return_pc?
The instruction before DW_AT_call_return_pc should be the call
instruction. Subtracting 1 will awkwardly put us in the middle of the
instruction, but it should at least map to the correct stack frame?
And without ISA-specific info it's the best we can do.
It's messy, but this should be all the info we need.
---
To build confidence in the new script, I included the --no-shrinkwrap
flag, which reverts to penalizing each call site for the function's
worst-case stack frame. This makes it easy to compare against the
-fcallgraph-info=su approach:
with -fcallgraph-info=su: 2624
with --dwarf=info --no-shrinkwrap: 2624
I was hoping that accounting for shrinkwrap-like optimizations would
reveal a lower stack cost, but for better or worse it seems that
worst-case stack usage is unchanged:
with --dwarf=info --no-shrinkwrap: 2624
with --dwarf=info: 2624
Still, it's good to know that our stack measurement is correct.
There is an argument for prefering nm for code size measurements due to
portability. But I'm not sure this really holds up these days with
objdump being so prevalent.
We already depend on objdump for ctx/structs/perf and other dwarf info,
so using objdump -t to get symbol information means one less tool to
depend on/pass around when cross-compiling.
As a minor benefit this also gives us more control over which sections
to include, instead of relying on nm's predefined t/r/d/b section types.
---
Note code.py/data.py did _not_ require objdump before this. They did use
objdump to map symbols to source files, but would just guess if
objdump wasn't available.
make ctx now does what you expect it to, and ctx.py now replaces
structs.py in the summary rules (make funcs, make summary):
$ make summary
... blablabla ...
code data stack ctx
TOTAL 38100 0 2624 752
Also finally cleaned up SUMMARYFLAGS in make funcs. This should have
been cleaned up when cleaning up make summary...
This showcases the sort of high-level result printing where -c/--compare
is useful:
$ make summary-diff
code data stack structs
BEFORE 57057 0 3056 1476
AFTER 68864 (+20.7%) 0 (+0.0%) 3744 (+22.5%) 1520 (+3.0%)
There was one hiccup though: how to hide the name of the first field.
It may seem minor, but the missing field name really does help
readability when you're staring at a wall of CLI output.
It's a bit of a hack, but this can now be controlled with -Y/--summary,
which has the sole purpose of disabling the first field name if mixed
with -c/--compare.
-c/--compare is already a weird case for the summary row anyways...
I still think the 24 (23+1) char minimum is a good default for 2 column
output such as help text, especially if you don't have automatic width
detection. But our result scripts need to be a bit more flexible.
Consider:
$ make summary
code data stack structs
TOTAL 68864 0 3744 1520
Vs:
$ make summary
code data stack structs
TOTAL 68864 0 3744 1520
Up until now we were just kind of working around this with cut -c 25- in
our Makefile, but now that our result scripts automatically scale the
table widths, they should really just default to whatever is the most
useful.
This seems like a more fitting name now that this script has evolved
into more of a general purpose high-level CSV tool.
Unfortunately this does conflict with the standard csv module in Python,
breaking every script that imports csv (which is most of them).
Fortunately, Python is flexible enough to let us remove the current
directory before imports with a bit of an ugly hack:
# prevent local imports
__import__('sys').path.pop(0)
These scripts are intended to be standalone anyways, so this is probably
a good pattern to adopt.
With the adoption of the odd-parity-zero rbyd perturb scheme, it's now
possible to validate individual tag's parity with neighboring valid
bits. This sparked an idea that I previously thought was intractable.
If we:
1. Validate all metadata reads by checking their on-disk parity bits.
2. Validate all data reads by checking their in-metadata checksums.
We end up with a closed system where all reads are checked by at least
a parity bit.
Being able to check all reads is a very valuable filesystem feature, but
difficult for littlefs:
- We need to keep relevant data in RAM while validating checksums.
We can't just validate checksums and then perform a second read as
that creates a hole where new bit-errors may be introduced.
- This is solved in other filesystems by loading and checking whole
blocks in RAM. We just can't do that here.
- Without parity, we would need to check the rbyd's checksum on every
tag read. This would lead to a crazy O(n^2 log n) rbyd compaction
runtime.
Which is why I original thought ckreads was just intractable.
Now, this isn't all sunshine and rainbows. ckreads, as implemented here,
has some deeply concerning flaws:
- A parity bit is, mathematically, the minimum possible error-detection
possible. Is validating reads with only a parity bit sufficient for
real world applications?
- Validating data checksums on every read may have severe performance
implications. We need to read up to the entire block, which can lead
to O(n^2) behavior when performing a lot of small reads in a file.
- In order to validate checksums/parity-bits, we need to know where the
checksums/parity-bits actually are for each piece of data.
Our lfsr_data_t struct provides a surprisingly nice abstraction for
this, but oof is it expensive.
For the added code/stack cost alone, we probably want to eventually make
this an opt-in compile-time feature.
---
Implementation notes:
- This found an actual compiler bug! Turns out increasing lfsr_data_t
from 3-words to 5-words confuses GCC:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101854
- Mid-commit, we may have not actually written the last tag's parity
yet, which is a bit of a problem because we may read the last tag when
building the next trunk!
Fixing this required a whole separate tailck mechanism, which just
tracks in-progress commit's parity bits.
This doesn't help the code/stack cost situation...
- lfsr_bd_read/cmp/cpy all need to be extended to support calculating a
checksum on the side, which is a bit of a mess.
- bptr's cksize/cksum is redundant now, which is going to make
conditional compilation a mess.
- The extra parity byte we need to read makes hint calculation a pain.
Code cost wise... yeah, it's significant. Turns out almost doubling
lfsr_data_t has a significant impact on stack usage. Add in all the
extra code to track checksums/parity-bits and validate checksums/
parity-bits and you got yourself a pretty heavy feature:
code stack
before: 36352 2672
after: 38100 (+4.8%) 3032 (+13.5%)
The original idea was to allow merging a whole bunch of different csv
results into a single lfs.csv file, but this never really happened. It's
much easier to operate on smaller context-specific csv files, where the
field prefix:
- Doesn't really add much information
- Requires more typing
- Is confusing in how it doesn't match the table field names.
We can always use summary.py -fcode_size=size to add prefixes when
necessary anyways.
Now, fractions are considered equal if they have the same ratio:
- 6/6 == 12/12 => True
- 3/6 == 3/12 => False
- 1/6 == 2/12 => True
It's interesting to note this implementation is actually more
numerically stable than float comparison, though that wasn't really the
goal.
The main reason for this is to allow other fields to take over when
sorting multi-field fractional data: cov (lines + branches), testmarks
(passed + time), etc. Before, sorting would usually stop after
mismatched fraction fields, which wasn't all that useful.
- Renamed build-test -> build-tests
- Renamed build-bench -> build-benches
- Added list-tests alias
- Added list-benches alias
Also made the Makefile's help text generation a bit more robust to long
rule names, which are common in Makefiles. If the name is >=21 chars, we
just indent, similar to test/bench_runner --help.
Note these are different than TESTFLAGS/BENCHFLAGS:
- TEST_CFLAGS/BENCH_CFLAGS => gcc $(TEST_CFLAGS) lfs.t.a.c -o lfs.t.a.o
- TESTFLAGS/BENCHFLAGS => ./scripts/test.py $(TESTFLAGS)
Also tried to group the src/tools/flags a bit better.
These provide useful file powerloss testing that scales linearly as long
as progress can be made. They can still struggle a bit, especially with
relocations which often fail to make progress, but they are _much_ better
than the O(n^2) simulation-based fuzz tests:
- test_files_pl_fuzz - 258734 pls
- test_relocations_pl_fuzz - 928638 pls
Our current problem with simulation-based fuzz testing is that we lose
the simulation on powerloss. We could brute force this, repeatedly
rerunning the simulation until it succeeds, but this grows O(n^2) with
our linear powerloss heuristic.
To avoid this, test_*_pl_fuzz doesn't bother with a simulation, instead
relying on internal asserts to catch bugs. This is less rigorous, but
realistically probably going to catch any powerloss related issues.
Some notes:
- We need to store some state on disk. If we don't we will still end up
with O(n^2) behavior because we simply don't know how many operations
we've accomplished so far.
- Since we rely on file operations to store our test state, this makes
this approach incompatible with the dir tests, which assume file
operations may not yet be implemented.
We still use O(n^2) powerloss testing in test_dirs, just with a small
number of directories.
- It's tempting to try to store a full simulation on disk. But you
would quickly run into atomicity issues with the simulation itself.
Powerloss resilience is tricky!
- We can at least store a checksum in the files (currently just mod 26)
to check that the file itself was not corrupted. This doesn't protect
against swapped data though.
---
Also, a bit of a tangent, but I needed to add -Wno-format-overflow to
the test flags to avoid an annoying invalid format-overlow warning:
struct lfs_info info;
char name[256];
if (strlen(info.name) < 100) { // can't overflow!?
sprintf(name, "test/%s", info.name); // <--
}
warning: '%s' directive writing up to 255 bytes into a region of size
251 [-Wformat-overflow=]
This seems like a GCC bug, because as far as I can tell there is no way
to signal or hint that the size is in bounds without just disabling the
warning completely...
The move to lfs_memcmp/lfs_strcmp highlighted an interesting hole in
prettyasserts.py: the lack of support for custom memcmp/strcmp symbols.
Rather than just adding more flags for an increasing number of symbols,
I've added -p/--prefix and -P/--prefix-insensitive to generate relevant
symbols based on a prefix. In littlefs's case, we use -Plfs_, which
matches both lfs_memcmp and LFS_ASSERT (and LFS_MEMCMP and lfs_assert
but ignore those):
$ ./scripts/prettyasserts.py -Plfs_ lfs.t.c -o lfs.t.a.c
Don't worry, you can still provide explicit symbols, but only via
long-form flags. This gets a bit noisy:
$ ./scripts/prettyasserts.py \
--assert=LFS_ASSERT \
--unreachable=LFS_UNREACHABLE \
--memcmp=lfs_memcmp \
--strcmp=lfs_strcmp \
lfs.t.c -o lfs.t.a.c
This commit also finally gives the prettyasserts.py's symbols actual
word boundaries, instead of the big error-prone hack of sorting by size.
- YES_COV -> COVGEN
- YES_PERF -> PERFGEN
- YES_PERFBD -> PERFBDGEN
- YES_TESTMARKS -> TESTMARKS
- YES_BENCHMARKS -> BENCHMARKS
For a lack of better naming, may change in the future. Couldn't really
find any consistent prior art.
Note that no-suf/prefix doesn't work because of conflicts with the tool
overrides (PERF, COV, etc).
- NO_COV -> YES_COV
- NO_PERF -> YES_PERF
- NO_PERFBD -> YES_PERFBD
Previously, COV defaulted to yes for tests, and PERFBD defaulted to yes
for benches. This is sometimes useful, but much less often than I
originally thought. Might as well not pay for what we don't use.
With this, the build features of the test/bench runners are consistent
by default, which is probably a good thing.
This _does_ have a noticable, if minor, impact on test runtime:
YES_COV: 674.15s
NO_COV: 584.97s (-13.2%)
As for the naming, the YES_* prefix is needed to avoid conflicts with
the tool variables themselves. I'm not sure what the best approach to
variable naming is here...
$ YES_PERF=1 PERF=~/my_perf/my_perf make test-runner -j
So instead of preprocessing lfs.t.a.c -> lfs.t.c, we preprocess
lfs.t.c -> lfs.t.a.c.
Really the extension should indicate what tool it was generated by, not
what tool it should be consumed by.
The previous commit that changed this states:
> Changed prettyasserts.py rule to .a.c => .c, allowing other .a.c files
> in the future.
But I'm not really sure why we would ever not just run prettyasserts.py
on every C file...
External use of prettyasserts.py made it clear the previous naming was a
bit weird.
Unfortunately, prettyasserts.py is having a hard time keeping up with
the constantly increasing number of tests. This is creating real
friction when debugging, as it now takes ~6x the time to preprocess
asserts as it does to actually compile the thing:
$ time ./scripts/prettyasserts.py \
-a LFS_ASSERT -u LFS_UNREACHABLE \
lfs.t.a.c -o lfs.t.c
real 0m16.187s
user 0m16.163s
sys 0m0.025s
$ time gcc -c -O0 -I. lfs.t.c -o lfs.o
real 0m2.466s
user 0m2.345s
sys 0m0.105s
Externally, I've rewritten prettyasserts.py in Rust, with more attention
towards performance (prettyasserts.py does quite a number of string
allocations). The result is quite satisfying:
$ time ~/prettyasserts/prettyasserts \
-a LFS_ASSERT -u LFS_UNREACHABLE \
lfs.t.a.c -o lfs.t.c
real 0m0.504s
user 0m0.464s
sys 0m0.040s
However, adding Rust as a requirement to test littlefs would be, uh,
quite a big jump.
So instead, littlefs keeps prettyassert.py, so only Python is needed out
of the box, and if the slow preprocessing is too much users are welcome
to provide their own prettyasserts binary via the PRETTYASSERTS env
variable:
$ time \
DEBUG=1 \
make test-runner -j
real 0m22.204s
user 0m44.841s
sys 0m1.478s
$ time \
DEBUG=1 PRETTYASSERTS=~/prettyasserts/prettyasserts \
make test-runner -j
real 0m5.699s
user 0m23.590s
sys 0m1.151s
The main benefit is control over error reporting and avoiding the dive
into stdlib layers when debugging thanks to __builtin_trap().
This changes -p/--pattern -> -a/--assert
And adds -u/--unreachable
I think ctags's defaults may have changed recently or something, but
it's struggling to find function definitions when there's a
predeclaration in the same file.
Adding --fields=+n (line numbers) fixes this.
So now instead of needing:
./scripts/test.py ./runners/test_runner test_dtree
You can just do:
./scripts/test.py test_dtree
Or with an explicit path:
./scripts/test.py -R./runners/test_runner test_dtree
This makes it easier to run the script manually. And, while there may be
some hiccups with the implicit relative path, I think in general this will
make the test/bench scripts easier to use.
There was already an implicit runner path, though only if the test suite
was completely omitted. I'm not sure that would ever have actually
been useful...
---
Also increased the permutation field size in --list-*, since I noticed it
was overflowing.
Christopher Haster