[review] Avoid infinite recursion in find_pc_sect_line

["Kevin Buettner (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>]

Kevin Buettner has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/682
......................................................................


Patch Set 1:

> Patch Set 1:
> 
> I'm not 100% comfortable in merging fixes like this, in an area that's already very tricky, without reproducer.  I'm not strongly opposing either, but just sharing my opinion.
> 
> It just doesn't sound logical.  To reach this "else", we need to:
> 
> 1. lookup a minsym for PC, that minsym is a trampoline.  It means that this trampoline minsym is the closest (smaller or equal) minsym from PC, all minsym types considered.
> 2. lookup a _text_ minsym by name, we find one that is exactly equal to pc
> 
> If that text minsym we have found in #2 is equal to PC, why did we not find it in #1?  The only way I see is for the two minsyms (the trampoline one and the text one) to have the same value.  The lookup function would have returned the trampoline one, because it can only return one symbol, so it has to choose, and the trampoline one happens to come before.  Then, we search specifically text symbols by name, we find the text one.  But that case would already handled with the existing
> 
> 	else if (BMSYMBOL_VALUE_ADDRESS (mfunsym)
> 		 == BMSYMBOL_VALUE_ADDRESS (msymbol))
> 
> So the only explanation I can imagine is that there was another GDB bug, causing it to mess up the minsym creation or lookup.  And either:
> 
> - that bug was fixed, and this fix is no longer needed
> - that bug still isn't fixed, in which case this fix just covers up the problem.
> 
> I think you are already conscious of this, since you added a warning to tell the user that this is not expected.  But if this is a condition we expect to never legitimately happen, I would suggest using an assertion instead:
> 
> 	    gdb_assert (BMSYMBOL_VALUE_ADDRESS (mfunsym) != pc);
> 	    return find_pc_line (BMSYMBOL_VALUE_ADDRESS (mfunsym), 0);
> 
> If this ever happens again in the field, we have more chances to hear about it than if it's a warning that users can easily overlook.

As a GDB user, I'd prefer that GDB do its best to provide a reasonable result. However, as I developer, I too, really want a bug report. I don't have a strong preference either way, so I went (mostly) with your suggestion, but the new patch calls internal_error instead of using an assert. This way, I could include a plea to file a bug report. (Though that may be obvious if you get an assertion failure.)

Anyway, the new patch can be found here:

https://www.sourceware.org/ml/gdb-patches/2019-12/msg00168.html

(I didn't use gerrit to post it.)


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I595470be6ab5f61ca7e4e9e70c61a252c0deaeaa
Gerrit-Change-Number: 682
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Buettner <kevinb@redhat.com>
Gerrit-Reviewer: Kevin Buettner <kevinb@redhat.com>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 04 Dec 2019 22:04:44 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment