This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Jim


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]