[CFI] Skip zeros when entry length is zero #486
Conversation
|
It'd be nice to figure out which compiler/linker is causing this and fix them too. Note that llvm-dwarfdump doesn't handle this kind of padding either. Also please squash the commits. |
Only skip null bytes when format is Dwarf32 Remove skip_null
I filed a bug for that: |
| return Ok(None); | ||
| } | ||
|
|
||
| // Hack: skip zero padding inserted by buggy compilers/linkers. |
There was a problem hiding this comment.
Technically speaking, skipping the zeros 4 by 4 works, but I don't think it's the right thing to do. What you actually have is not padding. What you have is multiple empty CIEs one after the other.
There was a problem hiding this comment.
Technically they are zero terminators + zero padding, not empty CIEs. And zero terminators are a .eh_frame thing, not a .debug_frame thing, so the bug is they shouldn't occur here. But why do you think that skipping them isn't the right thing to do? What should we do? Skipping them in .eh_frame would definitely be wrong, but I think it's okay to do it for .debug_frame?
There was a problem hiding this comment.
From the mozilla bug:
That's unfortunate, but not invalid. They "just" indicate empty records.
Where in the DWARF spec does it say these are valid?
There was a problem hiding this comment.
I'd ask the opposite question: where in the DWARF spec does it say they are invalid? There is no obvious mention of CIE length not being valid if it's 0. The spec doesn't say all fields must be there, and in fact, they can't since different versions of DWARF have different number of fields.
There was a problem hiding this comment.
Fair enough, but different versions must at least have a version field, and a length of 0 doesn't allow for that.
Also there is no point to have a zero length:
- it provides no benefit over just omitting it (not even for padding, because padding should be including in the CFI entry length and use
DW_CFA_nopinstead) .eh_framehandling requires you to stop processing after a zero length, so it's not even compatible with that
And given that multiple DWARF consumers don't handle it correctly, it's not at all obvious that it is valid. (You can add ubuntu's dwarfdump to the list of consumers that don't handle it.)
I don't think it's the right thing to do
What do you think should be done instead?
|
I filed a nasm bug: https://bugzilla.nasm.us/show_bug.cgi?id=3392657 |
Use the same trick as in readelf:
https://github.com/bminor/binutils-gdb/blob/master/binutils/dwarf.c#L7820
It aims to fix issue #485.