pfelf: load section names without mmap, and intern them - #1698
Conversation
| return err | ||
| } | ||
|
|
||
| nameIndexToSection := make(map[uint32]*Section, hdr.Shnum) |
There was a problem hiding this comment.
Should we keep track of the section?
| nameIndexToSection := make(map[uint32]*Section, hdr.Shnum) | |
| nameIndexToSection := make(map[uint32][]*Section, hdr.Shnum) |
I think, ELF files can have multiple sections that reference the same offset in shstrtab. Thinking about two .text sections produced by a linker script. The loop overwrites the map entry each time, so only the last *Section per uniquesh.Name offset is currently stored.
| rdr := pfbufio.NewReader(f.elfReader, int64(strsh.Offset), int64(strsh.Size)) | ||
| defer pfbufio.PutReader(rdr) | ||
|
|
||
| rdr.WalkAllStrings(func(offs int64, str string) error { |
There was a problem hiding this comment.
WalkAllStrings calls ReadString which advances the position in the buffer based on number of bytes read.
So the implicit assumption that WalkAllStrings makes is that strings are laid out cleanly one-after-another in memory, e.g.
FOO\0BAR\0BARBAR\0
But the indices into the section header string table don't require this, as one can have overlapping strings, e.g.
FOO\0BARBAR\0
See the example here.
There was a problem hiding this comment.
Yes, I was aware of this. But I real life this is not seen. Also sections we care about start with . and are not substrings of each other, so this cannot either happen.
If correctness is needed for the obscure/unused cases, we could sort the headers based on name index, and then PeekString() and Discard() the bytes until next string starts.
There was a problem hiding this comment.
OK, we can add a comment to make this obvious here for future reference.
There was a problem hiding this comment.
I'll need to think about this a bit further. Similar problem is with symbol names - and those probably need better solution than this. Probably makes sense to use same method for all string tables. Returning this to a draft state for now.
| rdr := pfbufio.NewReader(f.elfReader, int64(strsh.Offset), int64(strsh.Size)) | ||
| defer pfbufio.PutReader(rdr) | ||
|
|
||
| rdr.WalkAllStrings(func(offs int64, str string) error { |
There was a problem hiding this comment.
OK, we can add a comment to make this obvious here for future reference.
|
So I've been studying this and the symbol table layout, as getting symbol and dynamic symbol names are similar problem. It seems that the string tabs are as follows:
I'm currently thinking to just cap the sizes at 4 MB or so, and load them directly to a temporary byte buffer. And arrange the code so that if strings are kept, they either are interned or cloned so the kept strings will not refer to the big buffer. If the temporary peaks or memory usage, or the string tab size limitation becomes an issue, it can be improved in the future. The exceptions being likely the TLS visitor which likely should read individual strings (as it ever tries to read a few of them). |
Pull request dashboard statusWaiting on the author · refreshed 2026-09-02 16:31 UTC Move out of draft to request review. Status above doesn't look right?
|
3 participants
Memory and CPU benchmark of the coredump test suite indicates that interning the section names is a worthwhile action. The cost of interning is returned by reduced GC stress.
small step towards #1577