From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id B32243851C2F for ; Sat, 6 Jun 2020 08:18:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B32243851C2F Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tdevries@suse.de X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 41A50ABCE; Sat, 6 Jun 2020 08:18:30 +0000 (UTC) Subject: Re: [PATCH][gdb/symtab] Fix line-table end-of-sequence sorting To: Andrew Burgess Cc: gdb-patches@sourceware.org References: <51b43dba-2213-a428-d4dd-10154f8d1f52@suse.de> <446082ca-c3d4-1a90-2a35-2669c8407095@suse.de> <18b1ee90-2ece-a5b4-787b-2507b081da81@suse.de> <20200606065152.GF3522@embecosm.com> From: Tom de Vries Autocrypt: addr=tdevries@suse.de; keydata= xsBNBF0ltCcBCADDhsUnMMdEXiHFfqJdXeRvgqSEUxLCy/pHek88ALuFnPTICTwkf4g7uSR7 HvOFUoUyu8oP5mNb4VZHy3Xy8KRZGaQuaOHNhZAT1xaVo6kxjswUi3vYgGJhFMiLuIHdApoc u5f7UbV+egYVxmkvVLSqsVD4pUgHeSoAcIlm3blZ1sDKviJCwaHxDQkVmSsGXImaAU+ViJ5l CwkvyiiIifWD2SoOuFexZyZ7RUddLosgsO0npVUYbl6dEMq2a5ijGF6/rBs1m3nAoIgpXk6P TCKlSWVW6OCneTaKM5C387972qREtiArTakRQIpvDJuiR2soGfdeJ6igGA1FZjU+IsM5ABEB AAHNH1RvbSBkZSBWcmllcyA8dGRldnJpZXNAc3VzZS5kZT7CwKsEEwEIAD4WIQSsnSe5hKbL MK1mGmjuhV2rbOJEoAUCXSW0JwIbAwUJA8JnAAULCQgHAgYVCgkICwIEFgIDAQIeAQIXgAAh CRDuhV2rbOJEoBYhBKydJ7mEpsswrWYaaO6FXats4kSgc48H/Ra2lq5p3dHsrlQLqM7N68Fo eRDf3PMevXyMlrCYDGLVncQwMw3O/AkousktXKQ42DPJh65zoXB22yUt8m0g12xkLax98KFJ 5NyUloa6HflLl+wQL/uZjIdNUQaHQLw3HKwRMVi4l0/Jh/TygYG1Dtm8I4o708JS4y8GQxoQ UL0z1OM9hyM3gI2WVTTyprsBHy2EjMOu/2Xpod95pF8f90zBLajy6qXEnxlcsqreMaqmkzKn 3KTZpWRxNAS/IH3FbGQ+3RpWkNGSJpwfEMVCeyK5a1n7yt1podd1ajY5mA1jcaUmGppqx827 8TqyteNe1B/pbiUt2L/WhnTgW1NC1QDOwE0EXSW0JwEIAM99H34Bu4MKM7HDJVt864MXbx7B 1M93wVlpJ7Uq+XDFD0A0hIal028j+h6jA6bhzWto4RUfDl/9mn1StngNVFovvwtfzbamp6+W pKHZm9X5YvlIwCx131kTxCNDcF+/adRW4n8CU3pZWYmNVqhMUiPLxElA6QhXTtVBh1RkjCZQ Kmbd1szvcOfaD8s+tJABJzNZsmO2hVuFwkDrRN8Jgrh92a+yHQPd9+RybW2l7sJv26nkUH5Z 5s84P6894ebgimcprJdAkjJTgprl1nhgvptU5M9Uv85Pferoh2groQEAtRPlCGrZ2/2qVNe9 XJfSYbiyedvApWcJs5DOByTaKkcAEQEAAcLAkwQYAQgAJhYhBKydJ7mEpsswrWYaaO6FXats 4kSgBQJdJbQnAhsMBQkDwmcAACEJEO6FXats4kSgFiEErJ0nuYSmyzCtZhpo7oVdq2ziRKD3 twf7BAQBZ8TqR812zKAD7biOnWIJ0McV72PFBxmLIHp24UVe0ZogtYMxSWKLg3csh0yLVwc7 H3vldzJ9AoK3Qxp0Q6K/rDOeUy3HMqewQGcqrsRRh0NXDIQk5CgSrZslPe47qIbe3O7ik/MC q31FNIAQJPmKXX25B115MMzkSKlv4udfx7KdyxHrTSkwWZArLQiEZj5KG4cCKhIoMygPTA3U yGaIvI/BGOtHZ7bEBVUCFDFfOWJ26IOCoPnSVUvKPEOH9dv+sNy7jyBsP5QxeTqwxC/1ZtNS DUCSFQjqA6bEGwM22dP8OUY6SC94x1G81A9/xbtm9LQxKm0EiDH8KBMLfQ== Message-ID: Date: Sat, 6 Jun 2020 10:18:25 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: <20200606065152.GF3522@embecosm.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-16.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 06 Jun 2020 08:18:29 -0000 On 06-06-2020 08:51, Andrew Burgess wrote: > * Tom de Vries [2020-06-06 01:44:42 +0200]: > >> [ was: Re: [PATCH 2/3] gdb: Don't reorder line table entries too much >> when sorting. ] >> >> On 05-06-2020 18:00, Tom de Vries wrote: >>> On 05-06-2020 16:49, Tom de Vries wrote: >>>> On 23-12-2019 02:51, Andrew Burgess wrote: >>>>> I had to make a small adjustment in find_pc_sect_line in order to >>>>> correctly find the previous line in the line table. In some line >>>>> tables I was seeing an actual line entry and an end of sequence marker >>>>> at the same address, before this commit these would reorder to move >>>>> the end of sequence marker before the line entry (end of sequence has >>>>> line number 0). Now the end of sequence marker remains in its correct >>>>> location, and in order to find a previous line we should step backward >>>>> over any end of sequence markers. >>>>> >>>>> As an example, the binary: >>>>> gdb/testsuite/outputs/gdb.dwarf2/dw2-ranges-func/dw2-ranges-func-lo-cold >>>>> >>>>> Has this line table before the patch: >>>>> >>>>> INDEX LINE ADDRESS >>>>> 0 48 0x0000000000400487 >>>>> 1 END 0x000000000040048e >>>>> 2 52 0x000000000040048e >>>>> 3 54 0x0000000000400492 >>>>> 4 56 0x0000000000400497 >>>>> 5 END 0x000000000040049a >>>>> 6 62 0x000000000040049a >>>>> 7 END 0x00000000004004a1 >>>>> 8 66 0x00000000004004a1 >>>>> 9 68 0x00000000004004a5 >>>>> 10 70 0x00000000004004aa >>>>> 11 72 0x00000000004004b9 >>>>> 12 END 0x00000000004004bc >>>>> 13 76 0x00000000004004bc >>>>> 14 78 0x00000000004004c0 >>>>> 15 80 0x00000000004004c5 >>>>> 16 END 0x00000000004004cc >>>>> >>>>> And after this patch: >>>>> >>>>> INDEX LINE ADDRESS >>>>> 0 48 0x0000000000400487 >>>>> 1 52 0x000000000040048e >>>>> 2 END 0x000000000040048e >>>>> 3 54 0x0000000000400492 >>>>> 4 56 0x0000000000400497 >>>>> 5 END 0x000000000040049a >>>>> 6 62 0x000000000040049a >>>>> 7 66 0x00000000004004a1 >>>>> 8 END 0x00000000004004a1 >>>>> 9 68 0x00000000004004a5 >>>>> 10 70 0x00000000004004aa >>>>> 11 72 0x00000000004004b9 >>>>> 12 END 0x00000000004004bc >>>>> 13 76 0x00000000004004bc >>>>> 14 78 0x00000000004004c0 >>>>> 15 80 0x00000000004004c5 >>>>> 16 END 0x00000000004004cc >>>>> >>>>> When calling find_pc_sect_line with the address 0x000000000040048e, in >>>>> both cases we find entry #3, we then try to find the previous entry, >>>>> which originally found this entry '2 52 0x000000000040048e', >>>>> after the patch it finds '2 END 0x000000000040048e', which >>>>> cases the lookup to fail. >>>>> >>>>> By skipping the END marker after this patch we get back to the correct >>>>> entry, which is now #1: '1 52 0x000000000040048e', and >>>>> everything works again. >>>> >>>> I start to suspect that you have been working around an incorrect line >>>> table. >>>> >>>> Consider this bit: >>>> ... >>>> 0 48 0x0000000000400487 >>>> 1 52 0x000000000040048e >>>> 2 END 0x000000000040048e >>>> ... >>>> >>>> The end marker marks the address one past the end of the sequence. >>>> Therefore, it makes no sense to have an entry in the sequence with the >>>> same address as the end marker. >>>> >>>> [ dwarf doc: >>>> >>>> end_sequence: >>>> >>>> A boolean indicating that the current address is that of the first byte >>>> after the end of a sequence of target machine instructions. end_sequence >>>> terminates a sequence of lines; therefore other information in the same >>>> row is not meaningful. >>>> >>>> DW_LNE_end_sequence: >>>> >>>> The DW_LNE_end_sequence opcode takes no operands. It sets the >>>> end_sequence register of the state machine to “true” and appends a row >>>> to the matrix using the current values of the state-machine registers. >>>> Then it resets the registers to the initial values specified above (see >>>> Section 6.2.2). Every line number program sequence must end with a >>>> DW_LNE_end_sequence instruction which creates a row whose address is >>>> that of the byte after the last target machine instruction of the sequence. >>>> >>>> ] >>>> >>>> The incorrect entry is generated by this dwarf assembler sequence: >>>> ... >>>> {DW_LNS_copy} >>>> {DW_LNE_end_sequence} >>>> ... >>>> >>>> I think we should probably fix the dwarf assembly test-cases. >>>> >>>> If we want to handle this in gdb, the thing that seems most logical to >>>> me is to ignore this kind of entries. >>> >>> Hmm, that seems to be done already, in buildsym_compunit::record_line. >>> >>> Anyway, I was looking at the line table for >>> gdb.dwarf2/dw2-ranges-base.exp, and got a line table with subsequent end >>> markers: >>> ... >>> INDEX LINE ADDRESS IS-STMT >>> 0 31 0x00000000004004a7 Y >>> 1 21 0x00000000004004ae Y >>> 2 END 0x00000000004004ae Y >>> 3 11 0x00000000004004ba Y >>> 4 END 0x00000000004004ba Y >>> 5 END 0x00000000004004c6 Y >>> ... >>> >>> By using this patch: >>> ... >>> diff --git a/gdb/buildsym.c b/gdb/buildsym.c >>> index 33bf6523e9..76f0b54ff6 100644 >>> --- a/gdb/buildsym.c >>> +++ b/gdb/buildsym.c >>> @@ -943,6 +943,10 @@ buildsym_compunit::end_symtab_with_blockvector >>> (struct block *static_block, >>> = [] (const linetable_entry &ln1, >>> const linetable_entry &ln2) -> bool >>> { >>> + if (ln1.pc == ln2.pc >>> + && ((ln1.line == 0) != (ln2.line == 0))) >>> + return ln1.line == 0 ? true : false; > > I will take a look at this patch properly as soon as I can, but just > spotted this pet peeve of mine, please just write: > > return ln1.line == 0; Ack, thanks, will update that. Btw, I've retested this patch in combination with reverting the find_pc_sect_line part of "gdb: Don't reorder line table entries too much when sorting" and did not run into any test fails. Thanks, - Tom