[review] Avoid infinite recursion in find_pc_sect_line

["Simon Marchi (Code Review)" <gerrit at gnutoolchain-gerrit dot osci dot io>]

Simon Marchi has posted comments on this change.

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


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.


-- 
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, 27 Nov 2019 23:00:27 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment