Bug 29250 - readelf erases CIE initial register state
Summary: readelf erases CIE initial register state
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.39
: P2 normal
Target Milestone: 2.39
Assignee: Alan Modra
URL:
Keywords:
: 29268 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-06-15 12:56 UTC by Alan Modra
Modified: 2022-06-20 19:57 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alan Modra 2022-06-15 12:56:47 UTC
From: Vsevolod Alekseyev <sevaa@sprynet.com>

I'm debugging a DWARF parser library. We are testing it against GNU readelf, and we've found a discrepancy on the dump of the interpreted .eh_frame section of a particular x86_64 ELF binary.

The binary's first FDE in .eh_frame has initial_location 0x1060, and the
following instructions:
DW_CFA_advance_loc 4      # Move PC by 4
DW_CFA_undefined 16       # Change the rule for R16 to undefined

The linked CIE marks R16 as the return address, and has the following instructions:
DW_CFA_def_cfa 7, 8       # CFA is at R7+8)
DW_CFA_offset 16, 1       # Set the rule for R16 to [CFA+1*data_aligment_factor])


The GNU readelf, if executed with --debug-dump=frames-interp, dumps the FDE
as follows:

00000018 0000000000000014 0000001c FDE cie=00000000 pc=0000000000001060..0000000000001086
     LOC           CFA      ra
0000000000001060 rsp+8    u
0000000000001064 rsp+8    u


Meanwhile, the alternative parser thinks that at the range [0x1060-0x1064), the rule for RA/R16 should be as inherited from the CIE, and it goes rsp+8.

I've debugged readelf (the latest master, as of 06/01/22), to that point.  There are two passes over the FDE instructions: one starting on dwarf.c:9296, the other starting at dwarf.c:9442. On the first pass, when DW_CFA_undefined is encountered, there is the following case statement:

READ_ULEB (reg, start, block_end);
if (frame_need_space (fc, reg) >= 0)
  fc->col_type[reg] = DW_CFA_undefined;
break;

If I understand correctly, the intended purpose of the first pass is to allocate enough memory in the fc->col_type and fc->col_offset arrays, and the logic of this operator's handling was meant to be: if this register was not mentioned before, allocate space for it, and reset its rule to undefined. HOWEVER, if the register WAS mentioned before (e. g. in the CIE), frame_need_space() returns 0, and the if() body executes anyway, and resets the rule for the register to undefined, erasing the initial state as specified by the CIE.

I think the if statement should go, instead, "if (frame_need_space (fc, reg) > 0)". Same for other register-rule-type operators on the first pass.

The binary can be seen at
https://github.com/eliben/pyelftools/issues/409#issuecomment-1136720254
Comment 1 Alan Modra 2022-06-15 12:59:56 UTC
I agree that changing the test from >= 0 to > 0 will result in proper readelf behaviour, but I'm inclined to think that there is no need to set col_type at all in the first pass.
Comment 2 Alan Modra 2022-06-15 22:14:50 UTC
(In reply to Alan Modra from comment #1)
> I agree that changing the test from >= 0 to > 0 will result in proper
> readelf behaviour

Hmm, no it won't.  That might lose reg names from the header, or entries on lines of the --debug-dump=frames-interp display.
Comment 3 Sourceware Commits 2022-06-16 00:31:11 UTC
The master branch has been updated by Alan Modra <amodra@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=45bf072b34068b5a98947862b2aa183ab646e7ea

commit 45bf072b34068b5a98947862b2aa183ab646e7ea
Author: Alan Modra <amodra@gmail.com>
Date:   Wed Jun 15 22:30:51 2022 +0930

    PR29250, readelf erases CIE initial register state
    
            PR 29250
    binutils/
            * dwarf.c (display_debug_frames): Set col_type[reg] on sizing
            pass over FDE to cie->col_type[reg] if CIE specifies reg.
            Handle DW_CFA_restore and DW_CFA_restore_extended on second
            pass using the same logic.  Remove unnecessary casts.  Don't
            call frame_need_space on second pass over FDE.
    gas/
            * testsuite/gas/i386/ehinterp.d,
            * testsuite/gas/i386/ehinterp.s: New test.
            * testsuite/gas/i386/i386.exp: Run it.
Comment 4 Alan Modra 2022-06-16 00:40:01 UTC
Fixed for 2.39
Comment 5 Vsevolod Alekseyev 2022-06-20 19:57:25 UTC
*** Bug 29268 has been marked as a duplicate of this bug. ***