Re: PATCH: PR gas/10531: Strange assembler warning message on section group

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Mon, Oct 26, 2009 at 3:11 PM, Jim Wilson <wilson@codesourcery.com> wrote:
> On 08/22/2009 04:53 PM, H.J. Lu wrote:
>>
>> This patch hadles ".file" directives properly with a new testcase from gcc
>> 3.4.
>> OK to install?
>
> Now that I have time to properly review this...
>
> The stuff for excluding group sections from simple section name string
> searches looks mostly OK.
>
> I see that bfd_make_section_old_way contains the entire contents of the
> function bfd_get_section_by_name. ?This was OK when the latter was a 5-line
> function, but now that it is a 20-line function, maybe it is better to have
> bfd_make_section_old_way call bfd_get_section_by_name instead of duplicating
> all 20 lines? ?Except that would require some interface changes, because
> bfd_get_section_by_name can return NULL for two different things, one which
> bfd_make_section_old_way ignores, and one which needs special handling. ?So
> it is a little harder, but it still seems potentially worthwhile. ?Maybe
> part of this can be split out to its own function or macro?
>
> I see that you only fixed those two functions, but you left similar
> functions like bfd_make_section and bfd_make_section_with_flags unfixed.
> ?Maybe because these aren't called from gas/dwarf2dbg.c? ?Shouldn't we be
> fixing all of the functions? ?If we do need to fix more functions, then that
> means more copies of the same 20-lines of code, which makes it more
> important to avoid duplication of this code.
>
> I think the stuff for deciding whether to emit a debug_line section has more
> problems.
>
> There are some minor issues. ?For instance in as.c you added a comment
>>
>> /* If assembler shiuld debug info. ?*/
>
> which needs to be fixed. ?Need to change "shiuld" to "should generate".
>
> You changed the behaviour of the dwarf2_finish function, but you didn't
> update the comment before the function start which is now wrong with your
> patch.
>
> You added two variables gen_debug and dwarf2_directive_used, and a test for
> them
>>
>> + ?if (!dwarf2_directive_used && gen_debug == DEBUG_UNSPECIFIED)
>
> But this test seems to be equivalent to "if (!all_segs)" so there is no need
> for these two new variables.
>
> The bigger problem here is that with your patch dwarf2_finish now does
> ?if (!all_segs) return;
> ?...
> ?if (!all_segs && emit_other_sections) return;
> which doesn't make a lot of sense. ?The current code handles the case where
> the compiler emits a debug_info section, but does not emit any .file or .loc
> directives. ?.loc will only be emitted if there is code. It will not be
> emitted for an input file that contains only variables. GCC will always emit
> .file if the assembler supports it, but it is possible that it might be
> missing if someone configured gcc wrong. ?It isn't obvious whether we need
> to handle this case, it is probably safer if we do.
>
> This is why my suggested solution takes a slightly different tack here.
> ?Instead of checking for -g or .file/.loc, I check for a non-empty
> debug_line section. ?If there is one, then it should be safe to assume that
> the compiler emitted a correct debug_line section, and the assembler should
> not emit one. ?My suggested patch can be found here
> ?http://sourceware.org/ml/binutils/2009-08/msg00358.html
>

Here is the updated patch. OK to install?

Thanks.

-- 
H.J.
---
binutils/testsuite/

2009-10-27  H.J. Lu  <hongjiu.lu@intel.com>

	PR gas/10531
	* binutils-all/objdump.W: Remove bogus line debug info.

gas/

2009-10-27  Jim Wilson  <wilson@codesourcery.com>

	PR gas/10531
	* dwarf2dbg.c (dwarf2_finish): Don't generate .debug_line section
	if it isn't empty.

gas/testsuite/

2009-10-27  H.J. Lu  <hongjiu.lu@intel.com>

	 PR gas/10531
	 * gas/elf/dwarf2-1.d: New.
	 * gas/elf/dwarf2-1.s: Likewise.
	 * gas/elf/dwarf2-2.d: Likewise.
	 * gas/elf/dwarf2-2.s: Likewise.
	 * gas/elf/dwarf2-3.d: Likewise.
	 * gas/elf/dwarf2-3.s: Likewise.
	 * gas/i386/debug1.d: Likewise.
	 * gas/i386/debug1.s: Likewise.

Attachment: binutils-group-10.patch
Description: Text document