The idea, which has floated up a few times, is to add a third
representation of lfsr_data_t where the data is inlined in the struct
directly. In theory saving RAM for small pieces of data such as dids,
leb128s, flags, etc:
inlined: in-RAM buffer: on-disk:
.---+---+---+---. .---+---+---+---. .---+---+---+---.
|01| size | |00| size | |1| size |
+---+---+---+---+ +---+---+---+---+ +---+---+---+---+
| inlined data | | ptr -------. | block |
+ + +---+---+---+---+ | +---+---+---+---+
| | | (unused) | | | off |
'---+---+---+---' '---+---+---+---' | '---+---+---+---'
.---+---+---+---. |
| data |<'
: : :
Unfortunately in practice this just doesn't work out.
It turns out we benefit a lot from the _simplicity_ of lfsr_data_t. When
lfsr_data_t is built out of simple words, the compiler can make some
pretty strong assumptions and basically break it down into simple
register operations.
When you stick a byte array in the middle of the struct, this sort of
breaks down.
---
We can see this in our code measurements. After adding inlined data, but
before implementing slicing (in lfsr_data_fromslice), we can see decent
stack savings. But as soon as we add the memmove to lfsr_data_fromslice,
any benefit is lost:
code stack ctx
before: 38060 2608 752
without slicing: 38056 (-0.0%) 2568 (-1.5%) 752 (+0.0%)
after: 38128 (+0.2%) 2672 (+2.5%) 752 (+0.0%)
One reason for this is the extra logic does cause lfsr_data_fromslice to
be no longer inlined, but adding __attribute__((always_inline)) only
claws back some of the code/stack savings (though it's interesting to
note the compiler heuristic failure here):
code stack ctx
before: 38060 2608 752
after+inline: 38128 (+0.2%) 2672 (+2.5%) 752 (+0.0%)
after+always_inline: 38684 (+1.6%) 2656 (+1.8%) 752 (+0.0%)
---
Oh, and inlined lfsr_data_t is no longer compatible with LFSR_RAT's
simple data conversion, since lfsr_rat_t's can only point to existing
buffers. This causes tests to fail rather quickly.
This should be reverted, but I think the hidden cost of inlined
lfsr_data_t is surprising and interesting to note.
This was a disappointing failure of compount-literals.
These macros protect against mismatched buffer sizes, which is great for
preventing bugs caused by simple typos, but the overhead of compound-
literals requiring initialization make them simply unusable.
This commit leaves only a couple macros with implicit buffers:
LFSR_DATA_LEB128, and the LFSR_RAT_CAT/LFSR_RATS macros.
Even the tiny cleanup of the one remaining implicit-buffer macro still
in use, LFSR_DATA_GEOMETRY, saved some code:
code stack ctx
before: 38084 2608 752
after: 38060 (-0.1%) 2608 (+0.0%) 752 (+0.0%)
Well, renamed lfsr_rcompat_* really. But this avoids making rcompat
special and treats all rcompat/wcompat/ocompat logic as specializations
of the shared lfsr_compat_* logic.
No code changes:
code stack ctx
before: 38084 2608 752
after: 38084 (+0.0%) 2608 (+0.0%) 752 (+0.0%)
- Fixed issue where some overflowed compat flags could end up ignored.
A simple typo: incrementing by the unrelated d variable, meant we
were skipping overflowed compat flags whenever the previous logic sets
d > 1.
- Fixed issue where any zero padding was treated as overflowed compat
flags.
Note this hid the previous issue from our tests.
Added more tests to prevent a regression here. Letting bad compat flag
parsing through would be _very_ annoying in the future.
Code changes:
code stack ctx
before: 38148 2608 752
after: 38084 (-0.2%) 2608 (+0.0%) 752 (+0.0%)
This is to be consistent with other LFSR_DATA_* constructors. The code
is also a bit more readable when all LFSR_DATA_* constructors are
capitalized.
Not really sure why, but this saved a bit of stack? Probably just
compiler noise:
code stack ctx
before: 38144 2616 752
after: 38148 (+0.0%) 2608 (-0.3%) 752 (+0.0%)
I also explored adding in-place slice/truncate/fruncate functions as
well, but the impact on stack usage was REALLY BAD:
code stack ctx
by-value: 38148 2608 752
in-place: 38144 (-0.0%) 2688 (+3.1%) 752 (+0.0%)
I think maybe because the in-place functions end up with too many
pointers for the compiler to make safe assumptions about compound-
literal lifetimes?
Or what's left of lfsr_cat_t anyways.
lfsr_cat_t ended up being a pretty lfsr_rat_t specific optimization, so
it makes sense to drop the special type and simplify the code base a
little bit.
Curiously this trades some code for stack, I guess because inlining
static functions is a bit of a difficult heuristic mess:
code stack ctx
before: 38128 2624 752
after: 38144 (+0.0%) 2616 (-0.3%) 752 (+0.0%)
This reduces lfsr_ck_t to just the cksize/cksum fields, and moves all of
the compile-time ifdef LFS_CKDATACKSUMS logic up into the relevant
lfsr_data_* functions.
This doesn't solve the lfsr_data_t/lfsr_bptr_t duplication problem,
unfortunately, but does simplify the code base a bit.
No significant code changes:
code stack ctx
default before: 38128 2624 752
default after: 38128 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
ckdatacksums before: 39240 3008 752
ckdatacksums after: 39232 (-0.0%) 3008 (+0.0%) 752 (+0.0%)
To clarify this only checks data reads, and to makes space for future
theoretical ck-operations:
- ckmetaredund - likely
- ckdataredund - unlikely, expensive
- ckmetacksums - unlikely, expensive
- ckdatacksums - implemented
This also tweaks the relevant mount/format/info flags a bit:
LFS_M_CKPROGS 0x00100000 Check progs by reading back progged data
LFS_M_CKFETCHES 0x00200000 Check block checksums before first use
LFS_M_CKPARITY 0x00400000 Check metadata tag parity bits
LFS_M_CKMETAREDUND+ 0x01000000 Check metadata redund blocks on reads
LFS_M_CKDATAREDUND* 0x02000000 Check data redund blocks on reads
LFS_M_CKMETACKSUMS* 0x04000000 Check metadata checksums on reads
LFS_M_CKDATACKSUMS 0x08000000 Check data checksums on reads
+Planned
*Hypothetical
No code changes.
Unfortunately ckparity has proven itself to be much less useful than
originally thought.
The use of leb128 encoding in our tags means that ckparity can't even
detect single bit-errors reliably. Which raises the question: is
ckparity really worth all of the extra baggage necessary to track parity
in our codebase?
Fortunately we don't have to toss out ckparity entirely!
If we only check parity bits in lfsr_bd_readtag_, instead of on every
read, we still have a reasonable chance of noticing parity errors during
metadata lookups.
This does weaken ckparity, but allows us to drop a lot of lfsr_data_t's
ckparity baggage, at the cost of no longer, uh, unreliably detecting
parity errors during reads?
The limited error detection of ckparity means we can't reliably detect
errors during reads anyways, so we might as well keep the code/RAM/
maintenance implications at a minimum to make ckparity remotely worth
it.
---
Note the significant savings for both LFS_CKPARITY and LFS_CKCKSUMS.
Tracking parity info in lfsr_data_t had a heavy cost:
code stack ctx
default before: 38128 2624 752
default after: 38128 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
ckparity before: 39700 3048 760
ckparity after: 38476 (-3.1%) 2696 (-11.5%) 760 (+0.0%)
ckcksums before: 39396 3096 760
ckcksums after: 39240 (-0.4%) 3008 (-2.8%) 752 (-1.1%)
This also means lfsr_ck_ckprefix/cksuffix calls always have a ckoff of 0
(bptrs only), which means even more code savings, yay!
So... Long store short, checking metadata cksums is just intractably
slow.
But data cksums?
Yes checking data cksums is still O(b^2), but unlike metadata lookups,
which involve many small backwards reads, data reads are very easy to
cache. So instead of O(b^2), it's more like O(b^2/c), where c is your
rcache size.
Still O(b^2) when c << b, but I'm not sure that's avoidable without
adding more cksums.
At the very least, if you have enough RAM, c == b reduces this to O(b),
which is nice for "large" systems that want hardened reads without a
performance loss.
---
But why bother checking data cksums if we still have a read-hole with
metadata cksums?
Well, while considering the problem in the context of future features, I
noticed something _really interesting_:
- ckredund + metadata - reasonable ✓
- ckredund + data - impractical ✗, parity fanout + O(f+r) is bad
- ckcksums + metadata - impractical ✗, small reads + O(b^2) is bad
- ckcksums + data - reasonable ✓, assuming enough rcache
The current planned design for data redundancy makes it also intractably
slow to check every read, since it would require xoring all blocks that
contribute to the relevant parity block, but this isn't a problem for
metadata redundancy.
So while neither ckredund nor ckcksums can tractably close the read-hole
on their own, it looks like together they will be able to cover
everything without completely sacrificing performance. Neat!
Of course this isn't possible if ckcksums/ckredund imply checking both
metadata and data, so they need to be split apart.
And I don't really see a point in keeping the intractable variants
around in the codebase.
---
Dropping metadata ckcksums also means we can get rid of the ugly
lfsr_bd_ckrbydprefix and lfsr_bd_ckrbydsuffix functions, which were
basically duplicating all of lfsr_rbyd_fetch. That was quite a wart!
This saves a nice chunk of code when ckcksums is enabled:
code stack ctx
default before: 38128 2624 752
default after: 38128 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
ckparity before: 39724 3048 764
ckparity after: 39700 (-0.1%) 3048 (+0.0%) 760 (-0.5%)
ckcksums before: 40612 3184 772
ckcksums after: 39396 (-3.0%) 3096 (-2.8%) 760 (-1.6%)
In theory this gives callers more flexibility around what file states
they actually care about.
This didn't actually end up saving any code (I guess due to const
propagation?), but is kind of neat:
code stack ctx
before: 38128 2624 752
after: 38128 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
One of the unexpected side-effects of lazy file creation is that
suddenly LFS_O_EXCL doesn't make sense.
The standard definition: "Fail if the file exists", is easy enough to
implement, but doesn't really match what the user expects.
The user expects one of these calls to fail:
lfsr_file_open(&lfs, &file_a, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_open(&lfs, &file_b, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
But because we create files lazily (to prevent zero-length files after
powerloss), these both succeed.
---
I considered deferring the "file exists" check until we actually would
create the file, but while this _technically_ satisfies the
exclusitivity requirement, I decided against it as I think it just makes
the API way too confusing:
lfsr_file_open(&lfs, &file_a, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_open(&lfs, &file_b, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_close(&lfs, &file_a) => 0;
lfsr_file_close(&lfs, &file_b) => LFS_ERR_EXIST;
---
Instead, a simpler, more pragmatic approach: Fail if the file exists
_or_ if the file is open in a mode that will create the file:
lfsr_file_open(&lfs, &file_a, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_open(&lfs, &file_b, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => LFS_ERR_EXIST;
This explicitly does _not_ error on zombie/desync files:
lfsr_file_open(&lfs, &file_a, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_desync(&lfs, &file_a) => 0;
lfsr_file_open(&lfs, &file_b, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
And it does mean we aren't necessarily guaranteeing the file will be
created, but I think this does more-or-less what the user expects:
- open(a) -> desync(a) -> open(b) -> resync(a) is roughly equivalent to
opening a after creating b, which is perfectly fine with LFS_O_EXCL.
- open(a) -> open(b) (errors) -> desync(a) is one way to not actually
create the file, but is somewhat similar to removing the file after
creation.
If you're using desync files you should probably have a good
understanding of littlefs's sync model anyways.
And of course the user can always sync immediately after open to
guarantee file creation, while opting into the possibility of
zero-length files after powerloss.
Code changes:
code stack ctx
before: 38084 2624 752
after: 38128 (+0.1%) 2624 (+0.0%) 752 (+0.0%)
Making lfsr_tag_isunknown its own function suggests this is a cheap
operation, but it's not really since we need to check against all known
types.
Dropping lfsr_tag_isunknown and changing the relevant logic to fallback
to LFS_ERR_NOTSUP saves a surprising amount of code (ok, not that much,
but still surprising):
code stack ctx
before: 38140 2624 752
after: 38084 (-0.1%) 2624 (+0.0%) 752 (+0.0%)
This is the tradeoff of not erroring on unknown filetypes during mount.
- lfsr_file_open and lfsr_mtree_pathlookup now returns LFS_ERR_NOTSUP
instead of LFS_ERR_NOTDIR/LFS_ERR_ISDIR if it encounters an unkown
filetype.
This gets a bit subtle. You might think LFS_ERR_NOTDIR is reasonable,
but it's possible for our unknown filetype to be something dir-like.
Symlinks are an excellent example.
- lfsr_remove/lfsr_rename now bail with LFS_ERR_NOTSUP if encountering
an unknown filetype.
This conflicts with the POSIX philosophy of remove always being
allowed, but I'm not sure what other option there is. Maybe allowing
removes when mounted with LFS_M_FORCE?
We can't just allow removes by default because of the risk of leaking
resources. Directories being the main example of this (need to clean
up bookmarks).
Maybe leaky filetypes should also set WCOMPAT flags?
Not doing something is cheaper than doing something, so unfortunately
this costs us more than what we saved from dropping the orphan/unknown
scan during mount:
code stack ctx
bail: 38120 2624 725
no-error-no-bail (before): 38020 (-0.3%) 2624 (+0.0%) 752 (+0.0%)
error-no-bail (after): 38140 (+0.1%) 2624 (+0.0%) 752 (+0.0%)
But this is probably worth it for the extra flexibility.
The motivation here is to simplify lfsr_mount, but there's a number of
knock-on effects.
For one, lfsr_mount should now be faster on filesystems with large
blocks:
O(nb(log b)(log_b n)) -> O(nb(log_b n))
But we now no longer check if our filesystem contains orphaned
stickynotes or unknown filetypes:
- Orphaned stickynotes turned out to not be a big deal. If we find
orphans we'd need to do a second traversal to remove them anyways (no
mutation allowed in lfsr_mount), so this actually ends up a net
improvement in the found-orphan case.
If anything, doing a traversal on first write sets user expectations
correctly, and can be offloaded with lfsr_fs_mkconsistent or
lfsr_fs_gc.
- Unknown filetypes are a bit more annoying (I actually forgot about
this check), but unknown filetypes that require special care should
probably set WCOMPAT/RCOMPAT flags.
Allowing unknown filetypes is a bit more flexible in cases where a
filesystem image is being shared between drivers with different
features (bootloader + app for example).
Though we should probably add more checks/tests that we're handling
these correctly now that we no longer just bail during mount...
Also renamed LFS_I_HASORPHANS -> LFS_I_UNTIDY.
Not doing something is cheaper than doing something, so this saves a bit
of code:
code stack ctx
before: 38120 2624 752
after: 38020 (-0.3%) 2624 (+0.0%) 752 (+0.0%)
- test_attrs_fattr_zombie_no_receive
- test_attrs_fattr_mvrm_fuzz_fuzz
And renamed a number of broadcast tests to try to make it clear exactly
what we're testing:
- test_attrs_fattr_wronly_broadcast -> *_wronly_no_receive
- test_attrs_fattr_rdonly_broadcast -> *_rdonly_no_broadcast
- test_attrs_fattr_desync_broadcast -> *_desync_no_receive
- test_attrs_fattr_resync_broadcast -> *_resync_receive
- test_attrs_fattr_zombie_broadcast -> *_zombie_no_broadcast
As a part of the effort to undo the overuse of the term "orphan".
I can't really think of a better name, and uncreat gets the point
across. At least it matches LFS_O_UNSYNC/LFS_O_UNFLUSH.
Apparently the Uncreated are a race of aliens in the Marvel universe?
Unfortunately the import sys in the argparse block was hiding missing
sys imports.
The mistake was assuming the import sys in Python would limit the scope
to that if block, but Python's late binding strikes again...
Apparently __builtins__ is a CPython implementation detail, and behaves
differently when executed vs imported???
import builtins is the correct way to go about this.
I've been unhappy with LFSR_TAG_ORPHAN for a while now. While it's true
these represent orphaned files, they also represent zombied files. And
as long as a reference to the file exists in-RAM, I find it hard to say
these files are truely "orphaned".
We're also just using the term "orphan" for too many things.
Really this tag just represents an mid reservation. The term stickynote
works well enough for this, and fits in with the other internal tag,
LFSR_TAG_BOOKMARK.
We already have lfsr_cat_t so...
lfsr_rattr_t is a pretty fundamental type for littlefs, unfortunately
the name "rattr" is a mouthful. Shortening this to just "rat" hopefully
makes things easier to read at the cost of it being a bit less clear
what lfsr_rat_t actually is.
Though it's possible I've been staring at the dwarf spec (DW_AT_*) for
too long...
Moved local import hack behind if __name__ == "__main__"
These scripts aren't really intended to be used as python libraries.
Still, it's useful to import them for debugging and to get access to
their juicy internals.
Previously, we were using the checksum of the full path to generate
dids. This mostly works, but means that different paths can end up with
different dids for the same directory:
- crc32c("a/b") => 0xfb0b40a3
- crc32c("/a/c/../b") => 0x223404f6
Not the end of the world, but this is the stuff heisenbugs are made of.
Now we instead checksum only the file name, which doesn't have this
problem, and xor with our parent's did to prevent collisions between
same-named files in different directories:
did = parent_did xor crc32c(name)
As an extra plus, this should also trivially work for the theoretical
lfsr_mkdirat function.
Code size unchanged, which is humorous but not surprising:
code stack ctx
before: 38120 2624 752
after: 38120 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
This did not have any benefit code-size wise, and while it may be nice
for lfsr_mtree_pathlookup to take care of orphans, leaving it up to the
upper-layers is both simpler and makes orphan behavior explicit in all
functions.
We don't really have that many functions without special orphan
behavior anyways.
code stack ctx
before: 38124 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
after: 38120 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
This makes it so lfsr_mtree_pathlookup returns LFS_ERR_NOENT if it finds
an orphan, with lfsr_mtree_pathlookup_ providing the original behavior
of returning orphans as though they were normal files.
In theory the deduplication is nice, but in practice the overhead of
multiple function entry-points is just too much:
code stack ctx
before: 38124 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
after: 38124 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
Fortunately, while these two code bases have almost completely diverged
at this point, we can at least reuse the reworked test_paths tests.
Mostly involving corner-cases related to trailing-slashes, these changes
gives us better alignment with POSIX and hopefully fewer surprises for
users. The full details of what's changed is in the v2.10 release notes/
commits.
---
Implementing these changes here required a little bit of backpedaling.
Something that worked quite well upstream was the use of trailing junk
in the path to tell if a parent was not found, path must be dir, etc.
This is a bit more awkward with lfsr_mtree_pathlookup, with everything
taking an explicit name_size, but it greatly simplifies the mess that
was lfsr_mtree_pathlookup's error codes.
Now it's just:
- 0 => file found
- 0, lfsr_path_isdir(path) => dir found
- 0, mdir.mid=-1 => root found
- LFS_ERR_NOENT, lfsr_path_islast(path) => file not found
- LFS_ERR_NOENT, !lfsr_path_islast(path) => parent not found
- LFS_ERR_NOTDIR => parent not a dir
Note the special mdir.mid=-1 case for the root. This was needed since
lfsr_mtree_pathlookup can now return LFS_ERR_INVAL (for empty paths, dot
dots above root, etc).
In theory we could've gotten away with a different error code, but none
of them really make sense for this case.
---
The impact on code size is a bit funny. Modifying the path in-place _is_
a cheaper API, at the cost of being a bit more convoluted, but the extra
logic added for POSIX-alignment cancels this out:
code stack ctx
before: 38100 (-0.1%) 2624 (+0.0%) 752 (+0.0%)
after: 38120 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
Instead of trying to be too clever, this just adds a bunch of small
flags to control parts of table rendering:
- --no-header - Don't show the header.
- --small-header - Don't show by field names.
- --no-total - Don't show the total.
- -Q/--small-table - Equivalent to --small-header + --no-total.
Note that -Q/--small-table replaces the previous -Y/--summary +
-c/--compare hack, while also allowing a similar table style for
non-compare results.
This ended up being a pretty in-depth rework of prettyasserts.py to
adopt the shared Parser class. But now prettyasserts.py should be both
more robust and faster.
The tricky parts:
- The Parser class eagerly munches whitespace by default. This is
usually a good thing, but for prettyasserts.py we need to keep track
of the whitespace somehow in order to write it to the output file.
The solution here is a little bit hacky. Instead of complicating the
Parser class, we implicitly add a regex group for whitespace when
compiling our lexer.
Unfortunately this does make last-minute patching of the lexer a bit
messy (for things like -p/--prefix, etc), thanks to Python's
re.Pattern class not being extendable. To work around this, the Lexer
class keeps track of the original patterns to allow recompilation.
- Since we no longer tokenize in a separate pass, we can't use the
None token to match any unmatched tokens.
Fortunately this can be worked around with sufficiently ugly regex.
See the 'STUFF' rule.
It's a good thing Python has negative lookaheads.
On the flip side, this means we no longer need to explicitly specify
all possible tokens when multiple tokens overlap.
- Unlike stack.py/csv.py, prettyasserts.py needs multi-token lookahead.
Fortunately this has a pretty straightforward solution with the
addition of an optional stack to the Parser class.
We can even have a bit of fun with Python's with statements (though I
do wish with statements could have else clauses, so we wouldn't need
double nesting to catch parser exceptions).
---
In addition to adopting the new Parser class, I also made sure to
eliminate intermediate string allocation through heavy use of Python's
io.StringIO class.
This, plus Parser's cheap shallow chomp/slice operations, gives
prettyasserts.py a much needed speed boost.
(Honestly, the original prettyasserts.py was pretty naive, with the
assumption that it wouldn't be the bottleneck during compilation. This
turned out to be wrong.)
These changes cut total compile time in ~half:
real user sys
before (time make test-runner -j): 0m56.202s 2m31.853s 0m2.827s
after (time make test-runner -j): 0m26.836s 1m51.213s 0m2.338s
Keep in mind this includes both prettyasserts.py and gcc -Os (and other
Makefile stuff).
This was flipped in b5e264b.
Infering the type from the right-hand side is tempting here, but the
right-hand side if often a constant, which gets a bit funky in C.
Consider:
assert(lfs->cfg->read != NULL);
gcc: warning: ISO C forbids initialization between function pointer
and ‘void *’ [-Wpedantic]
assert(err < 0ULL);
gcc: warning: comparison of unsigned expression in ‘< 0’ is always
false [-Wtype-limits]
Prefering the left-hand type should hopefully avoid these issues most of
the time.
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?)
It's a bit funny, the motivation for a new Parser class came from the
success of simple regex + space munching in csv.py, but adopting Parser
in csv.py makes sense for a couple reasons:
- Consistency and better code sharing with other scripts that need to
parse things (stack.py, prettyasserts.py?).
- Should be more efficient, since we avoid copying the entire string
every time we chomp/slice.
Though I don't think this really matters for the size of csv.py's
exprs...
- No need to write every regex twice! Since Parser remembers the last
match.
If we're not using these results, no reason to collect all of the
children.
Note that we still need to recurse for other measurements (limit, struct
size, etc).
This has a measurable, but small, impact on runtime:
stack.py -z0 -Y: 0.202s
stack.py -z1 -Y: 0.162s (~-19.8%)
ctx.py -z0 -Y: 0.112s
ctx.py -z1 -Y: 0.098s (~-12.5%)
Now that cycle detection is always done at result collection time, we
don't need this in the table renderer itself.
This had a tendency to cause problems for non-function scripts (ctx.py,
structs.py).
God, I wish Python had an OrderedSet.
This is a fix for duplicate "cycle detected" notes when using -t/--hot.
This mix of merging both _hot_notes and _notes in the HotResult class is
tricky when the underlying container is a list.
The order is unlikely to be guaranteed anyways, when different results
with different notes are folded.
And if we ever want more control over the order of notes in result
scripts we can always change this back later.
- Error on no/insufficient files.
Instead of just returning no results. This is more useful when
debugging complicated bash scripts.
- Use elf magic to allow any file order in perfbd.py/stack.py.
This was already implemented in stack.py, now also adopted in
perfbd.py.
Elf files always start with the magic string "\x7fELF", so we can use
this to figure out the types of input files without needing to rely on
argument order.
This is just one less thing to worry about when invoking these
scripts.
It's been a while since I've been hurt by Python's late-binding
variables. In this case the scope-creep of the "file" variable hid that
we didn't actually know which recursive result belonged to which file.
Instead we were just assigning whatever the most recent top-level result
was.
This is fixed by looking up the correct file in childrenof. Though this
unfortunately does add quite a bit of noise.
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.
Problem: I misunderstood the purpose of .debug_frames (objdump
--dwarf=frames).
The purpose of .debug_frames is not to record the size of function
stack frames, but to only tell a debugger how to access the previous
function's stack frame. It just so happens that this _coincidentally_
tells you the stack frame size when compiling with -fomit-frame-pointer.
With -fno-omit-frame-pointer (common on some archs), .debug_frames just
says "hey here's the frame pointer" (DW_CFA_def_cfa_register), which
tells us nothing about the function's actual stack usage.
So unfortunately .debug_frames does not provide enough info on its
own...
---
This commit was an attempt to find the actual stack usage by looking at
the relevant variable info (DW_TAG_variable, etc) in function's dwarf
info, but this approach is also not looking very good...
1. The numbers do not appear correct:
before -fcallgraph-info=su: 2720
after --dwarf=info: 3558
after --dwarf=info --no-shrinkwrap: 3922
(this is with -fno-omit-frame-pointer)
In hindsight, this approach is fundamentally flawed. While the
variable tags does give us a lower bound on stack usage, it doesn't
tell us about implicit compiler variables and various stack push/pops
as a part of expression evaluation.
As far as I can tell there's simply not enough info in dwarf info to
find an accurate upper bound on stack usage.
2. This approach is quite a bit more complicated, since we need:
1. Dwarf info (--dwarf=info) to find variable tags.
2. Location info (--dwarf=loc) to map var allocations to address
ranges.
3. Range info (--dwarf=Ranges) to map lexical blocks to address
ranges when var allocation is implicit (not implemented).
4. And we still need frame info (--dwarf=frames)! since var
allocations are frame-relative.
3. Also dwarf info is not guaranteed to contain the whole callgraph.
It seems callgraph info is actually _omitted_ with -O0??
I guess this is because the callgraph info is a side-effect of some
compiler pass? This seems a bit backwards.
Dwarf does have a flag (DW_AT_call_all_calls) to indicate when
callgraph info is complete, but it doesn't seem to be set reliably?
Even with optimizations, lfsr_bd_sync, _and only lfsr_bd_sync_, is
missing the DW_AT_call_all_calls flag. I have no idea why. The flag
is still present in lfsr_bd_erase, lfsr_bd_read, and other functions
with function pointers...
So I think this will probably be reverted.
- Always interpret DW_AT_low_pc/high_pc/call_return_pc as hex.
Clang populates these with hex digits without a 0x prefix.
- Don't ignore callees with no name.
Now that DW_AT_abstract_origin is fixed, a callee with no name should
be an error.
- Prevented childrenof memoization from hiding the source of a
detected cycle.
- Deduplicated multiple cycle detected notes.
- Fixed note rendering when last column does not have a notes list.
Currently this only happens when entry is None (no results).
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.
We have symbol->addr info and dwarf->addr info (DW_AT_low_pc), so why
not use this to map symbols to dwarf entries?
This should hopefully be more reliable than the current name based
heuristic, but only works for functions (DW_TAG_subprogram).
Note that we still have to fuzzy match due to thumb-bit weirdness (small
rant below).
---
Ok. Why in Thumb does the symbol table include the thumb bit, but the
dwarf info does not?? Would it really have been that hard to add the
thumb bit to DW_AT_low_pc so symbols and dwarf entries match?
So, because of Thumb, we can't expect either the address or name to
match exactly. The best we can do is binary search and expect the symbol
to point somewhere _within_ the dwarf's DW_AT_low_pc/DW_AT_high_pc
range.
Also why does DW_AT_high_pc store the _size_ of the function?? Why isn't
it, idunno, the _high_pc_? I get that the size takes up less space when
leb128 encoding, but surely there could have been a better name?
Sometimes I feel like dwarf-info is designed to be as error-prone as
possible.
In this case, DW_AT_abstract_origin indicates that one dwarf entry
should inherit the attributes of another. If you don't know this, it's
easy to miss relevant dwarf entries due to missing name fields, etc.
Expanding DW_AT_abstract_origin lazily would be tricky due to how our
DwarfInfo class is structured, so instead I am just expanding
DW_AT_abstract_origins during collect_dwarf_info.
Note this doesn't handle recursive DW_AT_abstract_origins, but there is
at least an assert.
---
It does seem like DW_AT_abstract_origin is intended to be limited to
"Inline instances of inline subprograms" and "Out-of-line instances of
inline subprograms" according to the DWARF5 spec, but it's unclear if
this is a rule or suggestion...
This hasn't been an issue for existing scripts, but is needed from some
ongoing stack.py rework. Otherwise we don't find "out-of-line instances
of inline subprograms" (optimized functions?) correctly.
Long story short: DW_TAG_lexical_blocks are annoying.
In order to search the full tree of children of a given dwarf entry, we
need a recursive function somewhere. We might as well make this function
a part of the DwarfEntry class so we can share it with other scripts.
Note this is roughly the same as collect_dwarf_info, but limited to
the children of a given dwarf entry.
This is useful for ongoing stack.py rework.
Christopher Haster