[Bug other/63504] [5 Regression] Issues found by --enable-checking=valgrind
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63504 --- Comment #14 from ccoutant at google dot com --- > But then there is (question mainly on Cary) the .debug_types checksumming: > > case dw_val_class_const_double: > CHECKSUM_ULEB128 (DW_FORM_block); > CHECKSUM_ULEB128 (sizeof (at->dw_attr_val.v.val_double)); > CHECKSUM (at->dw_attr_val.v.val_double); > break; > > case dw_val_class_wide_int: > CHECKSUM_ULEB128 (DW_FORM_block); > CHECKSUM_ULEB128 (sizeof (*at->dw_attr_val.v.val_wide)); > CHECKSUM (*at->dw_attr_val.v.val_wide); > break; > > and that one ought to be governed by the DWARF standard, so I wonder how it > can > get away without converting for endianity etc. There shouldn't be any location lists in .debug_types, so I don't think the DWARF standard really matters for this case, and it would be reasonable to use the same hashing you use for other purposes. Some may slip through, unfortunately, so I covered this case just for completeness. -cary
[Bug debug/61013] [4.9/4.10 Regression] Option parsing difference between < 4.9 and 4.9
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61013 --- Comment #12 from ccoutant at google dot com --- > FWIW this regresses a few gdb tests. It's easy to fix the > gdb test suite, but if this is going to be fixed before the > next gcc release, I'd rather not bother. Any word on that? I'm planning to fix it as proposed above, and backport to the 4.9 branch -- I was just waiting to hear back from rth. -cary
[Bug debug/61013] [4.9/4.10 Regression] Option parsing difference between < 4.9 and 4.9
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=61013 --- Comment #10 from ccoutant at google dot com --- >> So, my preference would be to revert to the 4.8 and older behavior, or if >> there really is consensus that -g1 -g should mean -g2 rather than -g1, at >> least change it so that -g3 -g means -g3 (so revert your change and >> for *arg == '\0' instead of the 4.8: >> if (!opts->x_debug_info_level) >> opts->x_debug_info_level = DINFO_LEVEL_NORMAL; >> do: >> if (opts->x_debug_info_level < DINFO_LEVEL_NORMAL) >> opts->x_debug_info_level = DINFO_LEVEL_NORMAL; > > I agree on both points. Sorry, I'm not sure what "both points" are. Does that mean that you would support changing -g so that -g1 -g means -g2, but -g3 -g means -g3? (I.e., -g will raise the level to 2 but will not lower it.) That seems reasonable to me, and it would support both build scenarios mentioned above (Andres' and mine). It'll leave the meaning of -g3 -g the same as 4.8, but change the meaning of -g1 -g (which shouldn't be much of a problem since everyone here seemed to think that -g1 usage was rare). -cary
[Bug debug/55794] FAIL: g++.dg/debug/dwarf2/non-virtual-thunk.C -std=gnu++98 and -std=gnu++11
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55794 --- Comment #7 from ccoutant at google dot com --- > but the change is no longer in the current 4.9 code. Ah, right. See PR 54499 and this thread: http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00706.html -cary
[Bug rtl-optimization/57451] Incorrect debug ranges emitted for -freorder-blocks-and-partition -g
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57451 --- Comment #9 from ccoutant at google dot com --- >>> + if (!active_insn_p (insn)) >>> +continue; >> >> I'm not clear on why this is needed. Is it because after the >> change_scope, insn will now be a NOTE? If that's it, just put the >> continue in the previous if clause. > > Because the notes were being skipped by the iteration over > instructions, which previously only walked active instructions (notes > are not active instructions). So to see the switch section note I had > to walk all instructions, and just skip non-active instructions after > I am done checking for the note of interest. Oh, right. I didn't notice the change in the for loop. -cary
[Bug rtl-optimization/57451] Incorrect debug ranges emitted for -freorder-blocks-and-partition -g
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57451
--- Comment #7 from ccoutant at google dot com ---
> Index: final.c
> ===
> --- final.c (revision 201461)
> +++ final.c (working copy)
> @@ -1560,6 +1560,16 @@ change_scope (rtx orig_insn, tree s1, tree s2)
>tree ts1 = s1, ts2 = s2;
>tree s;
>
> + if (NOTE_P (orig_insn) && NOTE_KIND (orig_insn) ==
> NOTE_INSN_SWITCH_TEXT_SECTIONS)
> +{
> + gcc_assert (s1 == s1);
That should be s1 == s2, right?
> + rtx note = emit_note_before (NOTE_INSN_BLOCK_END, orig_insn);
> + NOTE_BLOCK (note) = s1;
> + note = emit_note_before (NOTE_INSN_BLOCK_BEG, next_insn (orig_insn));
> + NOTE_BLOCK (note) = s1;
> + return;
> +}
> +
>while (ts1 != ts2)
> {
>gcc_assert (ts1 && ts2);
> @@ -1604,12 +1614,16 @@ reemit_insn_block_notes (void)
>rtx insn, note;
>
>insn = get_insns ();
> - if (!active_insn_p (insn))
> -insn = next_active_insn (insn);
> - for (; insn; insn = next_active_insn (insn))
> + for (; insn; insn = next_insn (insn))
> {
>tree this_block;
>
> + if (NOTE_P (insn) && NOTE_KIND (insn) ==
> NOTE_INSN_SWITCH_TEXT_SECTIONS)
> +change_scope (insn, cur_block, cur_block);
It seems to me like you should just emit the three notes directly
here. This is really using change_scope for a new purpose, as you're
not really changing scopes, just putting a break into one.
> + if (!active_insn_p (insn))
> +continue;
I'm not clear on why this is needed. Is it because after the
change_scope, insn will now be a NOTE? If that's it, just put the
continue in the previous if clause.
-cary
[Bug other/46770] Replace .ctors/.dtors with .init_array/.fini_array on targets supporting them
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46770 --- Comment #87 from ccoutant at google dot com 2012-04-17 21:52:12 UTC --- > Just as a quick reminder, the reversed ctor execution order is big performance > problem for C++ Apps inlcuding Mozilla and Chrome ;) > So whatever we do, I would preffer to not have it by default. If the issue is the iteration over the contents of the final .init_array section, this solution won't have that problem -- the loader will still execute .init_array entries in forward order (we'll be reversing them at link time). If the issue is the code layout of the ctors themselves, that sounds like something that could be fixed through code layout optimizations (e.g., gold's --section-ordering-file option). -cary
[Bug other/46770] Replace .ctors/.dtors with .init_array/.fini_array on targets supporting them
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46770
--- Comment #86 from ccoutant at google dot com 2012-04-17 21:09:15 UTC ---
> I have seen codes like:
>
> void (*const init_array []) (void)
> __attribute__ ((section (".init_array"), aligned (sizeof (void * =
> {
> &init_0,
> &init_1,
> &init_2
> };
>
> I don't want to reverse its order.
We will not reverse that order. Those three entries will all be in the
same input section.
-cary
[Bug other/46770] Replace .ctors/.dtors with .init_array/.fini_array on targets supporting them
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46770 --- Comment #83 from ccoutant at google dot com 2012-04-17 20:10:07 UTC --- >> Didn't I just do that? > > Let me ask it again: > > The proposed --reverse-init-array switch will only reverse the order across > translation units, while keeping the same order within translation unit. > Is this correct? Maybe I'm misunderstanding the question. As I said before: If you have translation unit A with .ctors entries A1 and A2, and translation unit B with .ctors entries B1 and B2, we'll build a .init_array section with: B1 B2 A1 A2 Expanding on that, we will reverse the order of the individual input sections relative to one another, but we will not modify the contents of any input section at all. Everything I said for .ctors sections goes for .init_array sections, since we just map .ctors -> .init_array on the way in, and treat them as if they were .init_array sections from the beginning. Paul suggested to me offline that maybe you're asking about translation units with several .ctors or .init_array sections. Since that doesn't happen in practice, I don't really care so much, and would prefer to do the easy thing of just reversing the order of all input sections, even to the point of reversing the order of the sections within a translation unit. I think the important point is that we do not reverse the order of entries within a single input section. -cary
[Bug other/46770] Replace .ctors/.dtors with .init_array/.fini_array on targets supporting them
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46770 --- Comment #81 from ccoutant at google dot com 2012-04-17 18:52:11 UTC --- >> As Paul noted, this is a moot point in practice for .ctors, since GCC emits >> only a single .ctors entry per TU, but it could be significant for assembly >> code or for TUs with .init_array sections. > > That is my concern. .init_array section in the same TU can may have > more than one entry due to: > > 1. Assembly code. > 2. constructor attribute in C source. > 3. .init_array section attribute in C source. > > We need to spell out exactly what --reverse-init-array should do. Didn't I just do that? -cary
[Bug lto/47247] Linker plugin specification makes it difficult to handle COMDATs
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47247 --- Comment #20 from ccoutant at google dot com 2011-02-16 23:35:28 UTC --- > I have created a "small" test of the symbol table problem. It is in > > http://people.mozilla.com/~respindola/test.tar.xz > > The test is firefox's libxul with most files copied into one directory to make > it easy to run. The test script runs the link twice. First with the IL files > and then with combined .o file. > > The sizes are > > 39339456 libxul1.so > 34436696 libxul2.so > For a 5 MB reduction I might end up writing a wrapper that runs ld twice :-) That 5MB difference is all due to the dynamic symbol table? I think we can fix that in gold. : ) -cary
[Bug debug/46123] [4.5/4.6 Regression] ICE: in output_aranges, at dwarf2out.c:11531 with -feliminate-dwarf2-dups -g
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46123 --- Comment #4 from ccoutant at google dot com 2010-11-19 02:17:27 UTC --- > Ugh, this is very ugly. gen_subprogram_die sometimes decides to reuse old_die > which was DW_AT_declaration and can be deeply nested in type children, which > breaks -feliminate-dwarf2-dups as well as -gdwarf-4 if those are moved to > separate CUs (either the comdat ones or .debug_types). > The patch fixes this by not reusing the old die if doing one or another way of > duplicate removals (perhaps could do an extra check if the old_die is actually > in a tree that is going to be moved, which wouldn't be that hard to do for > break_out_includes, but would be uglier for .debug_types). Or we could do it > always. Unfortunately just doing that leads to crashes, because context_die > is > NULL and dwarf2out_finish doesn't want to see limbo DIEs with type contexts, > so > the patch also uses comp_unit_die () in that case. When would context_die == NULL in a case where it would not be OK to reuse the old_die? Wouldn't it be sufficient to force the new die only if context_die != NULL? -cary
