Re: [GOOGLE] Assign discriminators for different callsites at a same line within one BB
> This patch assigns discriminators for different callsites within the
> same BB. This is needed for accurate profile attribution in AutoFDO.
>
> Testing on going.
>
> OK for google branches if test pass?
> + for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> + {
> + gimple stmt = gsi_stmt (gsi);
> + if (gimple_code (stmt) != GIMPLE_CALL)
> +continue;
> + if (curr_locus == UNKNOWN_LOCATION ||
> + !same_line_p (curr_locus, gimple_location (stmt)))
> +curr_locus = gimple_location (stmt);
> + else
> +gimple_set_location (stmt, location_with_discriminator (
> + gimple_location (stmt),
> + next_discriminator_for_locus (gimple_location (stmt;
> + }
Please add a comment above this block explaining what it's doing. Are
you sure it's sufficient to just assign a new discriminator to the
CALL stmt, and not to the subsequent stmts after the CALL?
-cary
Re: [GOOGLE] Assign discriminators for different callsites at a same line within one BB
> You are right, we need discriminator for non-CALL stmts too. Patch updated: OK for google branches. Thanks! -cary
Re: [PING][PATCH] ICE with combination of -fopenmp and -femit-struct-debug-reduced/baseonly
> I’ve attached fix for this issue: > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57737 > There are fix, two tests and change log message. > > Is it ok? > > Cary, can you commit it for me? > > Ping this patch, http://gcc.gnu.org/ml/gcc-patches/2013-07/msg00053.html Thanks for the fix, and sorry for the delay! I've committed it for you at r202582. -cary 2013-09-13 Evgeny Gavrin gcc/ * dwarf2out.c (should_emit_struct_debug): Add check for type_decl variable is not NULL. gcc/testsuite * gcc.dg/debug/dwarf2/omp-fesdr.c: Add test. * g++.dg/debug/dwarf2/omp-fesdr.C: Add test.
MAINTAINERS: add myself as dwarf debugging code maintainer
2012-10-01 Cary Coutant * MAINTAINERS: Add myself as dwarf debugging code maintainer. Index: MAINTAINERS === --- MAINTAINERS (revision 191942) +++ MAINTAINERS (working copy) @@ -185,6 +185,7 @@ caller-save.c Jeff Law l...@redhat.com callgraph Jan Hubicka j...@suse.cz debugging code Jim Wilson wil...@tuliptree.org dwarf debugging code Jason Merrill ja...@redhat.com +dwarf debugging code Cary Coutantccout...@google.com c++ runtime libs Paolo Carlini paolo.carl...@oracle.com c++ runtime libs Gabriel Dos Reisg...@integrable-solutions.net c++ runtime libs Ulrich Drepper drep...@gmail.com
Re: PING^2: [patch] pr/54508: fix incomplete debug information for class
> /* We also have to mark its parents as used. > -(But we don't want to mark our parents' kids due to this.) */ > +(But we don't want to mark our parent's kids due to this, > +unless it is a class.) */ > if (die->die_parent) > - prune_unused_types_mark (die->die_parent, 0); > + prune_unused_types_mark (die->die_parent, > +(die->die_parent->die_tag == > DW_TAG_class_type || > + die->die_parent->die_tag == > DW_TAG_structure_type || > + die->die_parent->die_tag == > DW_TAG_union_type)); I'd suggest replacing these conditions with a call to class_scope_p(). That will also cover DW_TAG_interface_type, which might be irrelevant for this particular case, but is probably good to cover in the general case. -cary
Re: PING^2: [patch] pr/54508: fix incomplete debug information for class
> Index: gcc/testsuite/g++.dg/debug/dwarf2/localclass1.C
> ===
> --- gcc/testsuite/g++.dg/debug/dwarf2/localclass1.C (revision 192048)
> +++ gcc/testsuite/g++.dg/debug/dwarf2/localclass1.C (working copy)
> @@ -59,11 +59,11 @@
> // { dg-final { scan-assembler "foo\[^\n\r\]*DW_AT_name" } }
> // { dg-final { scan-assembler "staticfn1\[^\n\r\]*DW_AT_name" } }
> // { dg-final { scan-assembler "staticfn2\[^\n\r\]*DW_AT_name" } }
> -// { dg-final { scan-assembler-not "staticfn3\[^\n\r\]*DW_AT_name" } }
> -// { dg-final { scan-assembler-not "staticfn4\[^\n\r\]*DW_AT_name" } }
> +// { dg-final { scan-assembler "staticfn3\[^\n\r\]*DW_AT_name" } }
> +// { dg-final { scan-assembler "staticfn4\[^\n\r\]*DW_AT_name" } }
> // { dg-final { scan-assembler-not "staticfn5\[^\n\r\]*DW_AT_name" } }
> // { dg-final { scan-assembler-not "staticfn6\[^\n\r\]*DW_AT_name" } }
> -// { dg-final { scan-assembler-not "method1\[^\n\r\]*DW_AT_name" } }
> +// { dg-final { scan-assembler "method1\[^\n\r\]*DW_AT_name" } }
> // { dg-final { scan-assembler "arg1\[^\n\r\]*DW_AT_name" } }
> // { dg-final { scan-assembler "arg2\[^\n\r\]*DW_AT_name" } }
> // { dg-final { scan-assembler "arg3\[^\n\r\]*DW_AT_name" } }
The fact that these two tests were specifically checking for the
absence of staticfn3 and staticfn4 leads me to believe that the
current behavior is deliberate. Jakub, that change was yours (it dates
back to November 2008). Are you OK with Paul's change?
It seems to me that there are cases where we just want to emit the
class for the context info (like a namespace, which doesn't have to be
complete everywhere). Is there a way to tell the debugger that this
class declaration is incomplete and that it should look elsewhere for
a full definition?
-cary
Re: PING^2: [patch] pr/54508: fix incomplete debug information for class
> So given the comments, is this patch now ok to commit? Yes, this is OK. Thanks for doing the extra testing! (I also ran a quick test with -fdebug-types-section, just to make sure.) -cary
Re: PING^2: [patch] pr/54508: fix incomplete debug information for class
>> It seems to me that there are cases where we just want to emit the >> class for the context info (like a namespace, which doesn't have to be >> complete everywhere). Is there a way to tell the debugger that this >> class declaration is incomplete and that it should look elsewhere for >> a full definition? > > I think DW_AT_declaration would be appropriate. Yeah, that's what I was thinking. I was going to check with dje to see if GDB would do the right thing in that case. -cary
Re: PING^2: [patch] pr/54508: fix incomplete debug information for class
> There certainly is a fair amount of code in dwarf2read.c in gdb to handle > DW_AT_declaration and do things differently for declarations. > > Should I rework this patch to use that mechanism instead? If so, how? If > the class is marked only by prune_unused_types_mark visiting it as a parent, > but hasn't been marked by ??? that visits all its children, then emit it with > a DW_AT_declaration marking? One question I'd consider is what do you want to see in the debugger if this truly is the only debug info you have for the class? (For example, in the test case you added, a DW_AT_declaration attribute won't help if there's no full definition of the class anywhere else.) Is it reasonable to just show a truncated class definition in that case, or do you want the full definition available. My tentative answer would be that we do the pruning here because we expect there to be a full definition somewhere else, and that the lack of a DW_AT_declaration attribute is the bug. As you've discovered, however, it's not straightforward. You'll want to add the declaration attribute if you mark the DIE from below, but not from any reference where dokids is true. Alternatively, add the declaration attribute if any of its children are pruned. Perhaps that could be done in prune_unused_types_prune(). If you're willing to rework the patch this way (assuming GDB does the right thing with it), I think that would be better. Thanks. -cary
Re: [Dwarf Fission] Implement Fission Proposal (issue6305113)
>>> The potential savings here didn't seem worth the effort of adding a >>> pass over another table to assign slots in .debug_addr. In practice, >>> we're seeing very few slots zeroed out here. > > And how many duplicate entries? What strategy does Cary's patch use to > avoid those? I picked a compilation unit from one of our internal applications with one of the largest .debug_addr sections, and counted up the .debug_addr entries: Total count: 320,083 Unique entries: 8022 Unused entries (i.e., removed by resolve_addr): 13 Total refs to ".LVL" symbols: 313,554 Unique ".LVL" symbols: 3288 Entries after removing just duplicate ".LVL" refs: 9817 After the ".LVL" symbols, the most repeated entries were these: 298 .quad _ZdlPv 13 .quad _Znwm 12 .quad memmove Nearly all the remaining repeated entries were ".LC" (constant def) symbols (at most 6 of each). We have a lot of entries for ".LFB" (function begin) and ".LBB" (block begin) symbols, but no duplicates for any of these. I'm working on a follow-up patch to eliminate the duplicate references to ".LVL" symbols by keeping a direct-lookup table in dwarf2out_var_location. That will eliminate 310,266 of the 312,061 duplicates (99.4%) without using a hash table. With that patch, we'll have only a 22% overhead due to duplicate entries. (For this source file, at least -- I've seen similar numbers for a few other randomly-chosen sources, but I haven't done the same detailed analysis for them.) I'm also planning to do another later patch to generate location lists using the new DW_LLE_offset_pair_entry type for the .debug_locs.dwo section. This entry format uses offsets relative to the function's low_pc (or a base address selection entry), which (I think) will eliminate .debug_addr entries for ".LVL" symbols entirely. I think it should also get rid of most of the ".LFB" references as well. GDB isn't ready for that yet, so that one will come later. I'm trying to eliminate all .debug_addr entries that could be replaced by an offset relative to either another .debug_addr entry or an address in a range list. > I was thinking that the context of the reference would determine whether you > want a direct or indirect reference, in a way that would be clear when we go > to write out the reference. But if that isn't convenient, I don't mind > determining it when we build the reference. > > The added documentation for force_direct tells me what it means, but not > when you would want to pass true or false. What is the pattern here? We use force_direct when we're adding an attribute to a DIE in a skeleton compile unit or type unit (which will be in the .o file). It's false everywhere else (i.e., when the DIE is going into the .dwo file). -cary
Re: [PATCH] Improve expansion into DEBUG_IMPLICIT_PTR (PR debug/54970)
> 2012-10-18 Jakub Jelinek > > PR debug/54970 > * cfgexpand.c (expand_debug_expr): Expand &MEM_REF[&var, n] > as DEBUG_IMPLICIT_PTR + n if &var expands to DEBUG_IMPLICIT_PTR. > * tree-sra.c (create_access_replacement): Allow also MEM_REFs > with ADDR_EXPR first operand in DECL_DEBUG_EXPR expressions. > * var-tracking.c (track_expr_p): Handle MEM_REFs in DECL_DEBUG_EXPR > expressions. > * dwarf2out.c (add_var_loc_to_decl): Likewise. OK on the dwarf2out.c part. -cary
[patch] Fix PR 55063: Check whether DIE is already a declaration
I've committed the following fix for PR 55063: -cary 2012-10-25 Cary Coutant PR debug/55063 * dwarf2out.c (prune_unused_types_prune): Check whether DIE is already a declaration. Index: dwarf2out.c === --- dwarf2out.c (revision 192817) +++ dwarf2out.c (working copy) @@ -21259,7 +21259,8 @@ prune_unused_types_prune (dw_die_ref die /* If we pruned children, and this is a class, mark it as a declaration to inform debuggers that this is not a complete class definition. */ - if (pruned && die->die_mark == 1 && class_scope_p (die)) + if (pruned && die->die_mark == 1 && class_scope_p (die) + && ! is_declaration_die (die)) add_AT_flag (die, DW_AT_declaration, 1); }
[patch] Fix unresolved symbol with -gsplit-dwarf
When building the location list for a variable that has been optimized
by SRA, dw_sra_loc_expr sometimes creates a DWARF expression or a
piece of an expression, but then abandons it for some reason. When
abandoning it, we neglected to release any addr_table entries created
for DW_OP_addr_index opcodes, occasionally resulting in a link-time
unresolved symbol. This patch releases the addr_table entries before
abandoning a location expression.
Bootstrapped and regression tested on x86-64.
Committed to trunk at r203206.
-cary
2013-10-03 Cary Coutant
gcc/
* dwarf2out.c (dw_sra_loc_expr): Release addr_table entries when
discarding a location list expression (or a piece of one).
Index: dwarf2out.c
===
--- dwarf2out.c (revision 203183)
+++ dwarf2out.c (working copy)
@@ -13492,6 +13492,9 @@ dw_sra_loc_expr (tree decl, rtx loc)
if (last != NULL && opsize != bitsize)
{
padsize += bitsize;
+ /* Discard the current piece of the descriptor and release any
+addr_table entries it uses. */
+ remove_loc_list_addr_table_entries (cur_descr);
continue;
}
@@ -13500,18 +13503,24 @@ dw_sra_loc_expr (tree decl, rtx loc)
if (padsize)
{
if (padsize > decl_size)
- return NULL;
+ {
+ remove_loc_list_addr_table_entries (cur_descr);
+ goto discard_descr;
+ }
decl_size -= padsize;
*descr_tail = new_loc_descr_op_bit_piece (padsize, 0);
if (*descr_tail == NULL)
- return NULL;
+ {
+ remove_loc_list_addr_table_entries (cur_descr);
+ goto discard_descr;
+ }
descr_tail = &(*descr_tail)->dw_loc_next;
padsize = 0;
}
*descr_tail = cur_descr;
descr_tail = tail;
if (bitsize > decl_size)
- return NULL;
+ goto discard_descr;
decl_size -= bitsize;
if (last == NULL)
{
@@ -13547,9 +13556,9 @@ dw_sra_loc_expr (tree decl, rtx loc)
{
if (BYTES_BIG_ENDIAN != WORDS_BIG_ENDIAN
&& (memsize > BITS_PER_WORD || bitsize > BITS_PER_WORD))
- return NULL;
+ goto discard_descr;
if (memsize < bitsize)
- return NULL;
+ goto discard_descr;
if (BITS_BIG_ENDIAN)
offset = memsize - bitsize;
}
@@ -13557,7 +13566,7 @@ dw_sra_loc_expr (tree decl, rtx loc)
*descr_tail = new_loc_descr_op_bit_piece (bitsize, offset);
if (*descr_tail == NULL)
- return NULL;
+ goto discard_descr;
descr_tail = &(*descr_tail)->dw_loc_next;
}
}
@@ -13568,9 +13577,14 @@ dw_sra_loc_expr (tree decl, rtx loc)
{
*descr_tail = new_loc_descr_op_bit_piece (decl_size, 0);
if (*descr_tail == NULL)
- return NULL;
+ goto discard_descr;
}
return descr;
+
+discard_descr:
+ /* Discard the descriptor and release any addr_table entries it uses. */
+ remove_loc_list_addr_table_entries (descr);
+ return NULL;
}
/* Return the dwarf representation of the location list LOC_LIST of
[google/gcc-4_8] Backport fix for unresolved symbol with -gsplit-dwarf
When building the location list for a variable that has been optimized by SRA, dw_sra_loc_expr sometimes creates a DWARF expression or a piece of an expression, but then abandons it for some reason. When abandoning it, we neglected to release any addr_table entries created for DW_OP_addr_index opcodes, occasionally resulting in a link-time unresolved symbol. This patch releases the addr_table entries before abandoning a location expression. Backported from trunk r203206. Google ref b/10833306. 2013-10-03 Cary Coutant gcc/ * dwarf2out.c (dw_sra_loc_expr): Release addr_table entries when discarding a location list expression (or a piece of one).
Re: [Google 4.8 Patch] Generate gnu-pubnames for definitions only. Not declarations.
> 2013-10-09 Sterling Augustine > > * dwarf2out.c (include_pubname_in_output): Add conditional on > is_declaration_die > and debug_generate_pubnames. OK for google 4.8 branch. Thanks! -cary
Re: [PATCH] Do not append " *INTERNAL* " to the decl name
> It's hard to get a testcase without > http://gcc.gnu.org/viewcvs/gcc?view=revision&revision=201856 because > none of these *INTERNAL* symbols will be emitted in debug info. The original code was in there as a form of assembly-time assertion that the symbol would never get output. Now that you want to emit it in the debug info, the original intent is still valid, but without that extra kruft, the "assertion" is now gone. I think Jason is suggesting that you replace that with a test case, which would explicitly materialize one or more of the inlined functions that would get these mangled names, and then check the assembler output to verify that the (now valid) mangled name doesn't appear anywhere as an assembler label. -cary
Re: lto-plugin: mismatch between ld's architecture and GCC's configure --host
>> Ping. To sum it up, with these patches applied, there are no changes for >> a "regular" build (not using the new configure options). On the other >> hand, configuring GCC as described, it is possible use the 32-bit x86 >> linker for/with a x86_64 build, and get the very same GCC test results as >> when using a x86_64 linker. Sorry, I've been hoping someone with more global approval authority would respond. > Allow overriding the libiberty used for building the LTO plugin. > > lto-plugin/ > * configure.ac (--with-libiberty): New configure option. > * configure: Regenerate. > * Makefile.am (libiberty, libiberty_pic): New variables. > (liblto_plugin_la_LIBADD, liblto_plugin_la_LDFLAGS) > (liblto_plugin_la_DEPENDENCIES): Use them. > * Makefile.in: Regenerate. These look OK to me. > Allow for overriding a module's srcdir. > > * Makefile.tpl (configure-[+prefix+][+module+]) > (configure-stage[+id+]-[+prefix+][+module+]): Allow for > overriding a module's srcdir. > * Makefile.in: Regenerate. These look OK, but I think a global maintainer or build machinery maintainer should give approval. > Non-host system configuration for linker plugins. > > * configure.ac (--enable-linker-plugin-flags) > (--enable-linker-plugin-configure-flags): New flags. > (configdirs): Conditionally add libiberty-linker-plugin. > * configure: Regenerate. > * Makefile.def (host_modules): Add libiberty-linker-plugin. > (host_modules) : Pay attention to > @extra_linker_plugin_flags@ and > @extra_linker_plugin_configure_flags@. > (all-lto-plugin): Also depend on all-libiberty-linker-plugin. > * Makefile.in: Regenerate. > gcc/ > * doc/install.texi (--enable-linker-plugin-flags) > (--enable-linker-plugin-configure-flags): Document new flags. Same here. > GNU ld can use linker plugins, too. > > gcc/ > * doc/sourcebuild.texi (Top Level) : GNU ld can use > linker plugins, too. OK. > Fix typo. > > * Makefile.tpl: Fix typo. > * Makefile.in: Regenerate. OK (qualifies as trivial and obvious). -cary
[commit] Sync include/plugin-api.h with binutils
I'm committing the attached patch to sync include/plugin-api.h with binutils. -cary 2016-03-03 Than McIntosh * plugin-api.h: Add new hooks to the plugin transfer vector to to support querying section alignment and section size. (ld_plugin_get_input_section_alignment): New hook. (ld_plugin_get_input_section_size): New hook. (ld_plugin_tag): Add LDPT_GET_INPUT_SECTION_ALIGNMENT and LDPT_GET_INPUT_SECTION_SIZE. (ld_plugin_tv): Add tv_get_input_section_alignment and tv_get_input_section_size. 2016-03-03 Evgenii Stepanov * plugin-api.h (enum ld_plugin_tag): Add LDPT_GET_SYMBOLS_V3. plugin.patch Description: Binary data
Re: [Dwarf Patch] Use offset into debug_info for pubtype name referring to pubtype section
> gcc/ChangeLog > > 2013-12-02 Sterling Augustine > > * dwarf2out.c (output_pubnames): Use comp_unit_die ()->die_offset > when there > isn't a skeleton die. This is OK, but your patch also has a local change to contrib/mklog. Please be careful not to commit that. Thanks! -cary
Re: [PATCH] Use DW_LANG_D for D
> This patches gen_compile_unit_die to use the DW_LANG_D DWARF language > code for D. Is in relation to some other D language fixes that are > going to be submitted to gdb. Is this for a private front end? I'm not aware of any front ends that set the language name to "GNU D". Since it's so trivial, though, I have no problem with this patch for Stage 3 -- if you do have a separate front end that sets that language string, then it's arguably a bug fix. If this patch is preparation for more substantial changes to the GCC tree, however, I suspect you're going to need to wait for Stage 1 to reopen anyway. So, if this is a standalone patch, it's OK, but you also need a ChangeLog entry. -cary
Re: PR37132 – RFC patch for generation of DWARF symbol for Fortran's namelists (DW_TAG_namelist)
> gcc/ > 2013-11-24 Tobias Burnus > > PR debug/37132 > * lto-streamer.h (LTO_tags): Add LTO_namelist_decl_ref. > * tree.def (NAMELIST_DECL): Add. > * tree.h (NAMELIST_DECL_ASSOCIATED_DECL): New macro. > * tree.c (initialize_tree_contains_struct): Add asserts for it. > * dwarf2out.c (gen_namelist_decl): New function. > (gen_decl_die, dwarf2out_decl): Call it. > (dwarf2out_imported_module_or_decl_1): Handle NAMELIST_DECL. > * lto-streamer-in.c (lto_input_tree_ref): Handle NAMELIST_DECL. > (lto_input_tree_ref, lto_input_tree_1): Update lto_tag_check_range > call. > * lto-streamer-out.c (lto_output_tree_ref): Handle NAMELIST_DECL. > > gcc/fortran > 2013-11-24 Tobias Burnus > > PR debug/37132 > * trans-decl.c (generate_namelist_decl, create_module_nml_decl): > New static functions. > (gfc_generate_module_vars, generate_local_vars): Call them. > (gfc_trans_use_stmts): Handle namelists for debug genertion. The DWARF parts of this patch are OK with me. -cary On Sun, Nov 24, 2013 at 2:12 AM, Tobias Burnus wrote: > Hi all, > > attached is an updated version of the patch. > > Change: > > > Tobias Burnus wrote: >> >> But for "USE mod_name, only: nml", one is supposed to generate a >> DW_TAG_imported_declaration. And there I am stuck. For normal variables, the >> DW_TAG_imported_declaration refers to a DW_TAG_variable die. Analogously, >> for a namelist one would have to refer to a DW_TAG_namelist die. But such >> DW_TAG_namelist comes with a DW_TAG_namelist_item list. And for the latter, >> one needs to have the die of all variables in the namelist. But with >> use-only the symbols aren't use associate and no decl or die exists. >> (Failing call tree with the patch: gfc_trans_use_stmts -> >> dwarf2out_imported_module_or_decl_1 -> force_decl_die.) > > > With the attached patch, one now generates DW_TAG_namelist with no > DW_TAG_namelist_item and sets DW_AT_declaration. > > Thus, for (first file) > > module mm > > integer :: ii > real :: rr > namelist /nml/ ii, rr > end module mm > > > and (second file): > > subroutine test > use mm, only: nml > write(*,nml) > end subroutine test > > > One now generates (first file): > > <1><1e>: Abbrev Number: 2 (DW_TAG_module) > <1f> DW_AT_name: mm > <22> DW_AT_decl_file : 1 > <23> DW_AT_decl_line : 1 > <24> DW_AT_sibling : <0x59> > <2><28>: Abbrev Number: 3 (DW_TAG_variable) > <29> DW_AT_name: ii > <2c> DW_AT_decl_file : 1 > <2d> DW_AT_decl_line : 2 > <2e> DW_AT_linkage_name: (indirect string, offset: 0x15): __mm_MOD_ii > <32> DW_AT_type: <0x59> > <36> DW_AT_external: 1 > <36> DW_AT_location: 9 byte block: 3 0 0 0 0 0 0 0 0 (DW_OP_addr: > 0) > <2><40>: Abbrev Number: 3 (DW_TAG_variable) > <41> DW_AT_name: rr > <44> DW_AT_decl_file : 1 > <45> DW_AT_decl_line : 2 > <46> DW_AT_linkage_name: (indirect string, offset: 0x9): __mm_MOD_rr > <4a> DW_AT_type: <0x60> > <4e> DW_AT_external: 1 > <4e> DW_AT_location: 9 byte block: 3 4 0 0 0 0 0 0 0 (DW_OP_addr: > 4) > <2><58>: Abbrev Number: 0 > <1><59>: Abbrev Number: 4 (DW_TAG_base_type) > <5a> DW_AT_byte_size : 4 > <5b> DW_AT_encoding: 5(signed) > <5c> DW_AT_name: (indirect string, offset: 0x29): > integer(kind=4) > <1><60>: Abbrev Number: 4 (DW_TAG_base_type) > <61> DW_AT_byte_size : 4 > <62> DW_AT_encoding: 4(float) > <63> DW_AT_name: (indirect string, offset: 0x12c): > real(kind=4) > <1><67>: Abbrev Number: 5 (DW_TAG_namelist) > <68> DW_AT_name: nml > <2><6c>: Abbrev Number: 6 (DW_TAG_namelist_item) > <6d> DW_AT_namelist_items: <0x28> > <2><71>: Abbrev Number: 6 (DW_TAG_namelist_item) > <72> DW_AT_namelist_items: <0x40> > > Second file: > > <2><4f>: Abbrev Number: 3 (DW_TAG_imported_declaration) > <50> DW_AT_decl_file : 1 > <51> DW_AT_decl_line : 2 > <52> DW_AT_import : <0x70> [Abbrev Number: 6 (DW_TAG_namelist)] > <2><56>: Abbrev Number: 4 (DW_TAG_lexical_block) > <57> DW_AT_low_pc : 0xb > <5f> DW_AT_high_pc : 0xb0 > <2><67>: Abbrev Number: 0 > <1><68>: Abbrev Number: 5 (DW_TAG_module) > <69> DW_AT_name: mm > <6c> DW_AT_declaration : 1 > <6c> DW_AT_sibling : <0x76> > <2><70>: Abbrev Number: 6 (DW_TAG_namelist) > <71> DW_AT_name: nml > <75> DW_AT_declaration : 1 > <2><75>: Abbrev Number: 0 > > > Does the dumps look okay? For the first file, DW_TAG_namelist doesn't come > directly after DW_TAG_module but after its sibling 0x59; does one still see > that "nml" belongs to that module? (On dwarf2out level, context die should > point to the module tag, but I don't understand the readelf/eu-readelf > output well
Re: [PATCH] [GOLD] Add plugin API for processing plugin-added input files
> include/ChangeLog: > 2017-11-09 Stephen Crane > > * plugin-api.h: Add new plugin hook to allow processing of input > files added by a plugin. > (ld_plugin_new_input_handler): New funcion hook type. > (ld_plugin_register_new_input): New interface. > (LDPT_REGISTER_NEW_INPUT_HOOK): New enum val. > (tv_register_new_input): New member. > > > gold/ChangeLog: > 2017-11-09 Stephen Crane > > * plugin.cc (Plugin::load): Include hooks for register_new_input > in transfer vector. > (Plugin::new_input): New function. > (register_new_input): New function. > (Plugin_manager::claim_file): Call Plugin::new_input if in > replacement phase. > * plugin.h (Plugin::set_new_input_handler): New function. > * testsuite/plugin_new_section_layout.c: New plugin to test > new_input plugin API. > * testsuite/plugin_final_layout.sh: Add new input test. > * testsuite/Makefile.am (plugin_layout_new_file): New test case. > * testsuite/Makefile.in: Regenerate. These are OK. Thanks! Sri, I'm out of town through 11/18, and won't be able to commit the include/ patch to GCC before Stage 1 ends. Can you take care of it? (If not, I'll take care of it when I get back -- it was approved during Stage 1, so I think it's OK to commit early in Stage 3, especially since it's nothing but new declarations.) -cary
Re: [PATCH] Avoid excessive location expansion in assign_discriminators
> On testcases like that from PR60243 CFG build is dominated by > assign_discriminators because it expands locations again and again > and this got more expensive over the time. > > Cary - can you explain the overall logic of assign_discriminators, > specifically why if the last stmt of a block has UNKNOWN_LOCATION > we don't need any discriminator? That last stmt inherits the last > location from a previous stmt with location. Also why > do we look at e->dest _last_ stmt in addition to the first stmt? Sorry, it's been a long time since I looked at this. I think that test for UNKNOWN_LOCATION is just a way of punting on an unexpected condition; the rest of the function won't work unless we have a valid locus to start with. > If I understand correctly the assignment is done at CFG construction > time rather than location emission time because we want it done > on a representation close to source? So if the last stmt has > the same line then the first should have as well? As for why we look at last_stmt as well as first_stmt, that has to do with the way for loops are expanded. See my explanation here: https://gcc.gnu.org/ml/gcc-patches/2009-07/msg01450.html > Or, to ask a different question - why is this done so early on > a non-optimized CFG rather than late on RTL right before we > emit .loc directives? It has to be done early because we need to have discriminators assigned for the tree_profile pass, in order to use profile feedback from an earlier run. > It would be nice if you could expand the comment before > assign_discriminators in some way addressing the above. > > The patch below cuts the time spent in assign_discriminators > down by a factor of 2.5. There's obvious cleanup possibilities > for the pointer hash-table given we should rather embed the > int, int pair rather than allocating it on the heap. Couldn't > figure out the appropriate hash-traits quickly enough though. This looks good, except won't the hash table now compare equal for two different locations where the line is the same, but the file is not? In the Google branches, we improved discriminator assignment quite a bit by tracking per instruction instead of per basic block. Here's my original patch to do that: https://gcc.gnu.org/ml/gcc-patches/2009-11/msg00563.html Your improvement to hoist the expansion of locus_e would still be useful there. Unfortunately, I never had the chance to update that patch to preserve the discriminator info across LTO streaming, which is why it remained only in the Google branches. There were a few follow-on patches; the last backport to the google/gcc-4_9 branch combined most of them: https://gcc.gnu.org/ml/gcc-patches/2014-05/msg00869.html and I think there was one more discriminator patch after that: https://gcc.gnu.org/ml/gcc-patches/2014-08/msg02671.html -cary
Re: [PATCH] gold: Use autotools pthread macro
> The autotools library macro (AX_PTHREAD) is now used to detect if > pthreads is present and multi-threaded linking in gold is automatically > enabled if it is found. This enables multi-threaded gold on platforms > where pthreads is enabled via other methods than just -lpthread (e.g. > MinGW) > > Signed-off-by: Joshua Watt > --- > config/ax_pthread.m4 | 485 > gold/Makefile.am | 11 +- > gold/Makefile.in | 22 +- > gold/aclocal.m4| 1 + > gold/configure | 767 > +++-- > gold/configure.ac | 23 +- > gold/testsuite/Makefile.in | 8 +- > 7 files changed, 1254 insertions(+), 63 deletions(-) Thanks for the patch! I have a few preliminary questions and comments... First, do you or your company have a copyright assignment on file with FSF? I could be wrong, but I believe adding to config/ will require approval from a GCC config/ maintainer. The normal process, as I understand it, would be to add the file to the GCC tree, then sync it into the binutils tree. I'm not in a position to approve that, nor can I judge on the acceptability of the copyright notice in that file. I'd like to retain the ability to use --disable-threads as a configure option. If this is just to get MinGW on board, is there a lighter-weight way of doing that? Could we just add a configure option that switches between -lpthread and -pthread (or whatever option is needed)? I like the idea of fully automating it, but that's a pretty big m4 file! Anyway, I'd like to hear what the GCC folks think of adding ax_pthread.m4 to the config/ directory. (BTW, In the future, please omit diffs from generated files from your patch.) -cary
Re: [PATCH] gold: Use autotools pthread macro
>> First, do you or your company have a copyright assignment on file with FSF? > > I do not. What is the process for that? I don't need a company > assignment, an individual contributor for me will be fine. > > If I sign that for this project, would it also cover GCC, or do I need > one for each? It will cover all FSF contributions. If config/ax_pthread.m4 is accepted by the config maintainers, I doubt you'd need an assignment for that, since it's already licensed and it's not actually your contribution. That leaves a fairly small change to gold sources (not counting auto-regenerated files) -- small enough that I don't think you need to complete an assignment. If you do want to file a copyright assignment for future changes, I'll forward you a copy of the form and instructions separately. >> I could be wrong, but I believe adding to config/ will require >> approval from a GCC config/ maintainer. The normal process, as I >> understand it, would be to add the file to the GCC tree, then sync it >> into the binutils tree. I'm not in a position to approve that, nor can >> I judge on the acceptability of the copyright notice in that file. > > Ok. I didn't realize the config/ directory came from GCC. I'll look > into getting it submitted there How does that get "synced" to > binutils? Would I make a patch to add it here after it is added there? I've added config-patches to the conversation, so I'm hoping that was sufficient to get that going. Once added in the GCC tree, I think one can either wait for an automatic sync, or go ahead and submit a patch to do the sync (but I'm not completely versed on how the shared directories really operate). > FWIW, it is the same license as the ax_check_define macro (also from > the autotools macro archive) that is already in config/ That suggests to me that there shouldn't be a problem adding this new file. >> I'd like to retain the ability to use --disable-threads as a configure >> option. > > I will add that back in. Thanks. >> If this is just to get MinGW on board, is there a lighter-weight way >> of doing that? Could we just add a configure option that switches >> between -lpthread and -pthread (or whatever option is needed)? I like >> the idea of fully automating it, but that's a pretty big m4 file! > > It is pretty large I pulled it straight from the autoconf macro > archive. IMHO, its much better quality than anything I would be able > to come up with otherwise, and pretty brain-dead easy for the end > user. It should "just work" without any special switches (although the > user *can* configure it with some env-vars I think). Sounds good to me if others agree. >> (BTW, In the future, please omit diffs from generated files from your patch.) > > Will do. I couldn't find a lot of examples of config changes in > binutils, but I thought the one that I did updated the generated files > also. I must have been mistaken. It's a convention that makes reviewing and applying patches easier for us maintainers, but it's easy to forget to strip out those diffs. -cary
[config patch] Fwd from binutils: add ax_pthread.m4 to config/ directory
Please see this patch posted to the binutils list: https://sourceware.org/ml/binutils/2018-02/msg00260.html where Joshua proposes to add the ax_pthread.m4 script, from the autoconf macro archive, to the config/ directory in order to improve gold's configure-time detection of thread support. Is the config/ part of this patch OK? config/ * ax_pthread.m4: New file. -cary
Re: [PATCH] gold: Use autotools pthread macro
> config-patches has nothing to do with the GCC config/ directory. It is
> the place to send patches for config.{guess,sub}. Taking if off the
> cc: line.
Sorry, Ben. I've started a new thread on gcc-patches for the config/
part of this patch.
https://gcc.gnu.org/ml/gcc-patches/2018-02/msg01138.html
-cary
Re: plugin-api.h patch to add a new interface for linker plugins
> Ping. Is this alright to apply now or should I wait for Stage 1? > > * plugin-api.h (ld_plugin_get_wrap_symbols): New > plugin interface. I'd say go ahead and apply the patch in binutils, and wait for Stage 1 to sync back to GCC, unless someone there OKs it sooner. Nick, is that OK? -cary
Re: [GOOGLE, AUTOFDO] Assign different discriminators to calls with the same lineno
> static int
> -next_discriminator_for_locus (location_t locus)
> +increase_discriminator_for_locus (location_t locus, bool return_next)
> {
>struct locus_discrim_map item;
>struct locus_discrim_map **slot;
> @@ -934,8 +936,10 @@ next_discriminator_for_locus (location_t
>(*slot)->locus = locus;
>(*slot)->discriminator = 0;
> }
> +
>(*slot)->discriminator++;
> - return (*slot)->discriminator;
> + return return_next ? (*slot)->discriminator
> +: (*slot)->discriminator - 1;
> }
Won't this have the effect of sometimes incrementing the next
available discriminator without actually using the new value? That is,
if you call it once with return_next == false, and then with
return_next == true.
-cary
Remove skeleton type units that were being produced with -gsplit-dwarf.
This patch removes skeleton type units that were being produced
with -gsplit-dwarf. These sections were originally intended as
targets for .gdb_index entries that needed to point to type units.
Because of the limitations of the .debug_gnu_pubnames/pubtypes
sections with split DWARF, we were not able to pass along enough
information to the gold linker to generate those index entries
properly, and they had to point to the CU instead. GDB had to
deal with that, and was updated a while ago to no longer depend
on the skeleton TU sections at all. This allows us to reduce
object file sizes with split DWARF by about 30%.
I've run both GCC and GDB tests, and found no new regressions.
Committed as r213765.
-cary
2014-08-08 Cary Coutant
gcc/
* dwarf2out.c (get_skeleton_type_unit): Remove.
(output_skeleton_debug_sections): Remove skeleton type units.
(output_comdat_type_unit): Likewise.
(dwarf2out_finish): Likewise.
Index: gcc/dwarf2out.c
===
--- gcc/dwarf2out.c (revision 213764)
+++ gcc/dwarf2out.c (working copy)
@@ -9069,26 +9069,6 @@ add_top_level_skeleton_die_attrs (dw_die
add_AT_lineptr (die, DW_AT_GNU_addr_base, debug_addr_section_label);
}
-/* Return the single type-unit die for skeleton type units. */
-
-static dw_die_ref
-get_skeleton_type_unit (void)
-{
- /* For dwarf_split_debug_sections with use_type info, all type units in the
- skeleton sections have identical dies (but different headers). This
- single die will be output many times. */
-
- static dw_die_ref skeleton_type_unit = NULL;
-
- if (skeleton_type_unit == NULL)
-{
- skeleton_type_unit = new_die (DW_TAG_type_unit, NULL, NULL);
- add_top_level_skeleton_die_attrs (skeleton_type_unit);
- skeleton_type_unit->die_abbrev = SKELETON_TYPE_DIE_ABBREV;
-}
- return skeleton_type_unit;
-}
-
/* Output skeleton debug sections that point to the dwo file. */
static void
@@ -9127,8 +9107,6 @@ output_skeleton_debug_sections (dw_die_r
ASM_OUTPUT_LABEL (asm_out_file, debug_skeleton_abbrev_section_label);
output_die_abbrevs (SKELETON_COMP_DIE_ABBREV, comp_unit);
- if (use_debug_types)
-output_die_abbrevs (SKELETON_TYPE_DIE_ABBREV, get_skeleton_type_unit ());
dw2_asm_output_data (1, 0, "end of skeleton .debug_abbrev");
}
@@ -9190,38 +9168,6 @@ output_comdat_type_unit (comdat_type_nod
output_die (node->root_die);
unmark_dies (node->root_die);
-
-#if defined (OBJECT_FORMAT_ELF)
- if (dwarf_split_debug_info)
-{
- /* Produce the skeleton type-unit header. */
- const char *secname = ".debug_types";
-
- targetm.asm_out.named_section (secname,
- SECTION_DEBUG | SECTION_LINKONCE,
- comdat_key);
- if (DWARF_INITIAL_LENGTH_SIZE - DWARF_OFFSET_SIZE == 4)
-dw2_asm_output_data (4, 0x,
- "Initial length escape value indicating 64-bit DWARF extension");
-
- dw2_asm_output_data (DWARF_OFFSET_SIZE,
- DWARF_COMPILE_UNIT_HEADER_SIZE
- - DWARF_INITIAL_LENGTH_SIZE
- + size_of_die (get_skeleton_type_unit ())
- + DWARF_TYPE_SIGNATURE_SIZE + DWARF_OFFSET_SIZE,
- "Length of Type Unit Info");
- dw2_asm_output_data (2, dwarf_version, "DWARF version number");
- dw2_asm_output_offset (DWARF_OFFSET_SIZE,
- debug_skeleton_abbrev_section_label,
- debug_abbrev_section,
- "Offset Into Abbrev. Section");
- dw2_asm_output_data (1, DWARF2_ADDR_SIZE, "Pointer Size (in bytes)");
- output_signature (node->signature, "Type Signature");
- dw2_asm_output_data (DWARF_OFFSET_SIZE, 0, "Offset to Type DIE");
-
- output_die (get_skeleton_type_unit ());
-}
-#endif
}
/* Return the DWARF2/3 pubname associated with a decl. */
@@ -24430,7 +24376,6 @@ dwarf2out_finish (const char *filename)
skeleton die attrs are added when the skeleton type unit is
created, so ensure it is created by this point. */
add_top_level_skeleton_die_attrs (main_comp_unit_die);
- (void) get_skeleton_type_unit ();
htab_traverse_noresize (debug_str_hash, index_string, &index);
}
[google/gcc-4_9] Remove skeleton type units that were being produced with -gsplit-dwarf.
I've backported this patch from trunk r213765. These sections were originally intended as targets for .gdb_index entries that needed to point to type units. Because of the limitations of the .debug_gnu_pubnames/pubtypes sections with split DWARF, we were not able to pass along enough information to the gold linker to generate those index entries properly, and they had to point to the CU instead. GDB had to deal with that, and was updated a while ago to no longer depend on the skeleton TU sections at all. This allows us to reduce object file sizes with split DWARF by about 30%. Committed to the google/gcc-4_9 branch at r213768. -cary gcc/ * dwarf2out.c (get_skeleton_type_unit): Remove. (output_skeleton_debug_sections): Remove skeleton type units. (output_comdat_type_unit): Likewise. (dwarf2out_finish): Likewise.
Re: [GOOGLE, AUTOFDO] Assign different discriminators to calls with the same lineno
> To avoid the unused new discriminator value, I added a map
> "found_call_this_line" to track whether a call is the first call in a
> source line seen when assigning discriminators. For the first call in
> a source line, its discriminator is 0. For the following calls in the
> same source line, a new discriminator will be used everytime. The new
> patch is attached. Internal perf test and regression test are ok. Is
> it ok for google-4_9?
This seems overly complex to me. I'd think all you need to do is add a
bit to locus_discrim_map (stealing a bit from discriminator ought to
be fine) that indicates whether the next call should increment the
discriminator or not. Something like this:
increase_discriminator_for_locus (location_t locus, bool return_next)
{
...
if (return_next || (*slot)->needs_increment)
{
(*slot)->discriminator++;
(*slot)->needs_increment = false;
}
else
(*slot)->needs_increment = true;
return (*slot)->discriminator;
}
-cary
Re: [GOOGLE, AUTOFDO] Assign different discriminators to calls with the same lineno
2014-08-29 Wei Mi
* tree-cfg.c (struct locus_discrim_map): New field needs_increment.
(next_discriminator_for_locus): Increase discriminator only when
return_next or needs_increment are true.
(assign_discriminator): Add an actual for next_discriminator_for_locus.
(assign_discriminators): Assign different discriminators for calls
belonging to the same source line.
OK for google/gcc-4_9 branch. Thanks!
-cary
On Fri, Aug 29, 2014 at 1:59 PM, Wei Mi wrote:
>> On Fri, Aug 29, 2014 at 10:11 AM, Cary Coutant wrote:
>>>> To avoid the unused new discriminator value, I added a map
>>>> "found_call_this_line" to track whether a call is the first call in a
>>>> source line seen when assigning discriminators. For the first call in
>>>> a source line, its discriminator is 0. For the following calls in the
>>>> same source line, a new discriminator will be used everytime. The new
>>>> patch is attached. Internal perf test and regression test are ok. Is
>>>> it ok for google-4_9?
>>>
>>> This seems overly complex to me. I'd think all you need to do is add a
>>> bit to locus_discrim_map (stealing a bit from discriminator ought to
>>> be fine) that indicates whether the next call should increment the
>>> discriminator or not. Something like this:
>>>
>>> increase_discriminator_for_locus (location_t locus, bool return_next)
>>> {
>>> ...
>>> if (return_next || (*slot)->needs_increment)
>>> {
>>> (*slot)->discriminator++;
>>> (*slot)->needs_increment = false;
>>> }
>>> else
>>> (*slot)->needs_increment = true;
>>> return (*slot)->discriminator;
>>> }
>>>
>>> -cary
>
> Here is the new patch (attached). Regression test passes. Cary, is it ok?
>
> Thanks,
> Wei.
Re: PR37132 – RFC patch for generation of DWARF symbol for Fortran's namelists (DW_TAG_namelist)
> But for "USE mod_name, only: nml", one is supposed to generate a > DW_TAG_imported_declaration. > > And there I am stuck. For normal variables, the DW_TAG_imported_declaration > refers to a DW_TAG_variable die. Analogously, for a namelist one would have > to refer to a DW_TAG_namelist die. But such DW_TAG_namelist comes with a > DW_TAG_namelist_item list. And for the latter, one needs to have the die of > all variables in the namelist. But with use-only the symbols aren't use > associate and no decl or die exists. (Failing call tree with the patch: > gfc_trans_use_stmts -> dwarf2out_imported_module_or_decl_1 -> > force_decl_die.) > > What's the proper DWARF way of handling this? Creating a DW_TAG_namelist > without any DW_TAG_namelist_items, relying on the debugger to pick those > from the module? Or how is one supposed to handle it? Why wouldn't you have DIEs for the imported namelist items? I'd think that once the compiler has processed the USE statement and imported the namelist into the current compilation unit, it would generate DIEs for all the imported items, and then be able to construct a fully-populated DW_TAG_namelist DIE. (At least it would have DIEs for all the imported items that are actually used in that module, which should be sufficient for debugging.) -cary
Re: [PATCH] Generate a label for the split cold function while using -freorder-blocks-and-partition
>>> Is there a format for compiler-defined labels that would not be able >>> to clash with other user-generated labels? >> >> My understanding is that the "." in the generated name ensures that it >> will not clash with user-generated labels which cannot contain ".". So >> this should not be an issue. > > Is that true for all languages? I don't think we can make that > assumption. Also, there may be clashes with other compiler generated > symbols. I know of at least one Fortran compiler that composes names > for module symbols as "modulename.symbolname", and if symbolname is > "cold" then you can have a clash. GCC already uses "." to generate names for cloned functions, and picks a different separator for targets where "." is legal (see clone_function_name in cgraphclones.c). It would probably be a good idea to use clone_function_name to generate the new name (in particular, because the C++ demangler already knows how to handle that style when it's applied to mangled names). -cary
Re: [PATCH] Generate a label for the split cold function while using -freorder-blocks-and-partition
>> Isn't this something that should be expressed in DWARF with >> DW_AT_ranges? See DWARF4, section 2.17, >> >> Does GCC generate such ranges? > > GCC does generate these ranges. However, according to Cary many tools > do not rely on dwarf info for locating the corresponding function > name, they just look at the symbols to identify what function an > address resides in. Nor would we want tools such as objdump and > profilers to rely on dwarf for locating the function names as this > would not work for binaries that were generated without -g options or > had their debug info stripped. Yes, while the information needed is in the DWARF info, I don't think it's a good idea to depend on having debug info in all binaries. It's quite common to need to symbolize binaries that don't have debug info, and without a symbol such as Sri and Teresa are proposing, the result will be not just an address that didn't get symbolized, but an address that gets symbolized incorrectly (in a way that will often be quite misleading). -cary
Re: PR37132 – RFC patch for generation of DWARF symbol for Fortran's namelists (DW_TAG_namelist)
> One cannot simply also import, e.g., "i" as the code might have: > > subroutine example() > use mod, only: my_nml > integer :: i! < Locally defined variable > read(uid, my_nml) > ... > end subroutine example > > In that case "i" is the local variable. As written, one can create a decl > and a die for the "i" of the module, but the question is how to name it. Ah, sorry, I didn't understand how the use statement works with a namelist. I thought that when you import a namelist, it imports all the names in that namelist individually, and that the read(..., my_nml) was equivalent to read(..., ii, rr). This sounds like a good question for the DWARF workgroup. Could you forward this thread to dwarf-disc...@lists.dwarfstd.org? We have some Fortran experts on that list, but I'm not one of them. -cary
[patch] Fix ICEs when DEBUG_MANGLE is enabled
This patch fixes a few ICEs I encountered when enabling DEBUG_MANGLE.
I've also changed dump_substitution_candidates to output the substitution
index in base 36, to match the actual mangled name.
OK for trunk?
-cary
2013-11-13 Cary Coutant
gcc/
* cp/mangle.c (to_base36): New function.
(dump_substitution_candidates): Add checks for NULL.
Print substitution index in base 36.
commit 5ece725d55f104dd6499ac261380a9c9c4002613
Author: Cary Coutant
Date: Wed Nov 13 09:28:58 2013 -0800
Fix ICEs when DEBUG_MANGLE is enabled.
diff --git a/gcc/cp/mangle.c b/gcc/cp/mangle.c
index 202fafc..56c1844 100644
--- a/gcc/cp/mangle.c
+++ b/gcc/cp/mangle.c
@@ -301,6 +301,19 @@ decl_is_template_id (const tree decl, tree* const
template_info)
return 0;
}
+/* Convert VAL to base 36. */
+
+static const char *
+to_base36 (int val)
+{
+ static char buffer[sizeof (HOST_WIDE_INT) * 8 + 1];
+ unsigned count;
+
+ count = hwint_to_ascii (number, 36, buffer + sizeof (buffer) - 1, 1);
+ buffer[sizeof (buffer) - 1] = '\0';
+ return buffer + sizeof (buffer) - 1 - count;
+}
+
/* Produce debugging output of current substitution candidates. */
static void
@@ -317,12 +330,27 @@ dump_substitution_candidates (void)
if (i > 0)
fprintf (stderr, "");
if (DECL_P (el))
- name = IDENTIFIER_POINTER (DECL_NAME (el));
+{
+ if (DECL_NAME (el))
+name = IDENTIFIER_POINTER (DECL_NAME (el));
+}
else if (TREE_CODE (el) == TREE_LIST)
- name = IDENTIFIER_POINTER (DECL_NAME (TREE_VALUE (el)));
+{
+ tree val = TREE_VALUE (el);
+ if (TREE_CODE (val) == IDENTIFIER_NODE)
+name = IDENTIFIER_POINTER (val);
+ else if (DECL_P (val))
+name = IDENTIFIER_POINTER (DECL_NAME (val));
+}
else if (TYPE_NAME (el))
- name = IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (el)));
- fprintf (stderr, " S%d_ = ", i - 1);
+{
+ if (DECL_NAME (TYPE_NAME (el)))
+name = IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (el)));
+}
+ if (i == 0)
+fprintf (stderr, " S_ = ");
+ else
+fprintf (stderr, " S%s_ = ", to_base36 (i - 1));
if (TYPE_P (el) &&
(CP_TYPE_RESTRICT_P (el)
|| CP_TYPE_VOLATILE_P (el)
Re: [PATCH, MPX, 2/X] Pointers Checker [20/25] Debug info
> 2013-11-15 Ilya Enkovich > > * dbxout.c (dbxout_type): Ignore POINTER_BOUNDS_TYPE. > * dwarf2out.c (gen_subprogram_die): Ignore bound args. > (gen_type_die_with_usage): Skip pointer bounds. > (dwarf2out_global_decl): Likewise. The dwarf2out.c changes are OK. I'm not a dbx maintainer, but I'd suggest that your patch to that file is trivial and obvious. -cary
[patch] PR 59195: C++ demangler handles conversion operator incorrectly
In PR 59195, I describe a demangler problem with parsing
conversion operators that have template parameters:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59195
For example,
$ c++filt _ZN1AcvPT_I1CEEv
fails -- it should print "A::operator C*()".
$ c++filt _ZN1AcvT_IiEI1CEEv
prints "A::operator int()" instead of
"A::operator C()".
This patch fixes the parser to resolve the ambiguity in the mangled
name grammar where it can't tell if the "T_" is a
or is the start of , and
fixes the printer to use the correct context when resolving the
template parameter substitution.
Passes all regression tests, and I've added new test cases.
OK to commit?
-cary
2013-11-19 Cary Coutant
libiberty/
PR other/59195
* cp-demangle.c (struct d_info_checkpoint): New struct.
(struct d_print_info): Add current_template field.
(d_operator_name): Set flag when processing a conversion
operator.
(cplus_demangle_type): When processing for
a conversion operator, backtrack if necessary.
(d_expression_1): Renamed from d_expression.
(d_expression): New wrapper around d_expression_1.
(d_checkpoint): New function.
(d_backtrack): New function.
(d_print_init): Initialize current_template.
(d_print_comp): Set current_template.
(d_print_cast): Put current_template in scope for
printing conversion operator name.
(cplus_demangle_init_info): Initialize is_expression and
is_conversion.
* cp-demangle.h (struct d_info): Add is_expression and
is_conversion fields.
* testsuite/demangle-expected: New test cases.
diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c
index 7be9804..edfd85c 100644
--- a/libiberty/cp-demangle.c
+++ b/libiberty/cp-demangle.c
@@ -287,6 +287,19 @@ struct d_saved_scope
struct d_print_template *templates;
};
+/* Checkpoint structure to allow backtracking. This holds copies
+ of the fields of struct d_info that need to be restored
+ if a trial parse needs to be backtracked over. */
+
+struct d_info_checkpoint
+{
+ const char *n;
+ int next_comp;
+ int next_sub;
+ int did_subs;
+ int expansion;
+};
+
enum { D_PRINT_BUFFER_LENGTH = 256 };
struct d_print_info
{
@@ -318,6 +331,8 @@ struct d_print_info
struct d_saved_scope *saved_scopes;
/* Number of saved scopes in the above array. */
int num_saved_scopes;
+ /* The nearest enclosing template, if any. */
+ const struct demangle_component *current_template;
};
#ifdef CP_DEMANGLE_DEBUG
@@ -444,6 +459,10 @@ d_add_substitution (struct d_info *, struct
demangle_component *);
static struct demangle_component *d_substitution (struct d_info *, int);
+static void d_checkpoint (struct d_info *, struct d_info_checkpoint *);
+
+static void d_backtrack (struct d_info *, struct d_info_checkpoint *);
+
static void d_growable_string_init (struct d_growable_string *, size_t);
static inline void
@@ -1734,8 +1753,15 @@ d_operator_name (struct d_info *di)
if (c1 == 'v' && IS_DIGIT (c2))
return d_make_extended_operator (di, c2 - '0', d_source_name (di));
else if (c1 == 'c' && c2 == 'v')
-return d_make_comp (di, DEMANGLE_COMPONENT_CAST,
- cplus_demangle_type (di), NULL);
+{
+ struct demangle_component *type;
+
+ if (!di->is_expression)
+di->is_conversion = 1;
+ type = cplus_demangle_type (di);
+ di->is_conversion = 0;
+ return d_make_comp (di, DEMANGLE_COMPONENT_CAST, type, NULL);
+}
else
{
/* LOW is the inclusive lower bound. */
@@ -2284,13 +2310,61 @@ cplus_demangle_type (struct d_info *di)
ret = d_template_param (di);
if (d_peek_char (di) == 'I')
{
- /* This is . The
- part is a substitution
+ /* This may be .
+If this is the type for a conversion operator, we can
+have a here only by following
+a derivation like this:
+
+
+->
+->
+->
+->
+->
+-> cv
+-> cv
+
+where the is followed by another.
+Otherwise, we must have a derivation like this:
+
+
+->
+->
+->
+->
+->
+-> cv
+-> cv
+
+where we need to leave the to be processed
+by d_prefix (following the ).
+
+The part is a substitution
candidate. */
- if (! d_add_substitution (di, ret))
- return NULL;
- ret = d_make_comp (di, DEMANGLE_COMPONENT_TEMPLATE, ret,
-d_template_args (di));
+ if (! di->is_conv
Re: [patch] Fix ICEs when DEBUG_MANGLE is enabled
> 2013-11-13 Cary Coutant > > gcc/cp/ > * mangle.c (to_base36): New function. > (dump_substitution_candidates): Add checks for NULL. > Print substitution index in base 36. Ping? -cary
Re: [RFC] Modify -g1 to produce line tables
>> Here, finally, is that patch again, reworked to generate line tables >> at -g1. I plan to commit this when Stage 1 reopens, but I'd like to >> verify that earlier consensus. I also plan to commit this to the >> google/main branch, and future merges will go more smoothly if what I >> put in google/main matches what eventually goes into trunk. > > Hmm, Stage 1 has been opened for a while now but I could not find > this patch has been committed yet. Is there any plans to include this > patch? It would be useful for Sanitizer and other uses. Sorry, I never saw any feedback, positive or negative, on that, and it kind of fell off my radar. I think it should still be ready to go in -- Stage 1 is still open for another day, right? Let me rebase the patch, kick off a bootstrap and regression tests, and I think I can be ready to submit it if there are no objections. -cary
Re: [RFC] Modify -g1 to produce line tables
>> Sorry, I never saw any feedback, positive or negative, on that, and it >> kind of fell off my radar. I think it should still be ready to go in >> -- Stage 1 is still open for another day, right? Let me rebase the >> patch, kick off a bootstrap and regression tests, and I think I can be >> ready to submit it if there are no objections. > > Yea, you've still got another day. So definitely rebase and resubmit. > > From a review standpoint, things got really bad. If you had something get > dropped, definitely ping it. As a whole, we're doing better now than > probably anytime this year, but as always we could do better. Sorry, I didn't mean to make it sound like I was blaming any potential reviewers -- it fell off of my radar (even though I got reminded occasionally about it by Dehao Chen, who really wants this in trunk). Totally my failure. -cary
Re: [RFC] Modify -g1 to produce line tables
> Let me rebase the
> patch, kick off a bootstrap and regression tests, and I think I can be
> ready to submit it if there are no objections.
Here's the rebased patch. I'm running the bootstrap and regression tests now.
-cary
commit 2d50b3878cd8e96e31b92a5f1d261cbcea6ce0e5
Author: Cary Coutant
Date: Wed Nov 20 16:02:11 2013 -0800
Add minimal line tables at -g1.
2013-11-20 Cary Coutant
gcc/
* dwarf2out.c (want_pubnames): Don't do pubnames for -g1.
(add_linkage_name): Don't add linkage name for -g1.
(decls_for_scope): Process subblocks for -g1.
(dwarf2out_source_line): Output line tables for -g1.
(dwarf2out_finish): Likewise.
* tree-ssa-live.c (remove_unused_scope_block_p): Don't prune
unused scopes for -g1.
* opts.c (common_handle_option): Handle -g same as -g2.
* doc/invoke.texi: Update description for -g1.
gcc/testsuite/
* gcc.dg/debug/dwarf2/mlt1.c: New test.
* gcc.dg/debug/dwarf2/mlt2.c: New test.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 6fc56b9..0a26212 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -5233,8 +5233,8 @@ Level 0 produces no debug information at all. Thus,
@option{-g0} negates
Level 1 produces minimal information, enough for making backtraces in
parts of the program that you don't plan to debug. This includes
-descriptions of functions and external variables, but no information
-about local variables and no line numbers.
+descriptions of functions and external variables, and line number
+tables, but no information about local variables.
Level 3 includes extra information, such as all the macro definitions
present in the program. Some debuggers support macro expansion when
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 23cd726..1c0effd 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -8849,6 +8849,8 @@ output_comp_unit (dw_die_ref die, int output_if_empty)
static inline bool
want_pubnames (void)
{
+ if (debug_info_level <= DINFO_LEVEL_TERSE)
+return false;
if (debug_generate_pub_sections != -1)
return debug_generate_pub_sections;
return targetm.want_debug_pub_sections;
@@ -16563,11 +16565,12 @@ add_src_coords_attributes (dw_die_ref die, tree decl)
static void
add_linkage_name (dw_die_ref die, tree decl)
{
- if ((TREE_CODE (decl) == FUNCTION_DECL || TREE_CODE (decl) == VAR_DECL)
- && TREE_PUBLIC (decl)
- && !DECL_ABSTRACT (decl)
- && !(TREE_CODE (decl) == VAR_DECL && DECL_REGISTER (decl))
- && die->die_tag != DW_TAG_member)
+ if (debug_info_level > DINFO_LEVEL_TERSE
+ && (TREE_CODE (decl) == FUNCTION_DECL || TREE_CODE (decl) == VAR_DECL)
+ && TREE_PUBLIC (decl)
+ && !DECL_ABSTRACT (decl)
+ && !(TREE_CODE (decl) == VAR_DECL && DECL_REGISTER (decl))
+ && die->die_tag != DW_TAG_member)
{
/* Defer until we have an assembler name set. */
if (!DECL_ASSEMBLER_NAME_SET_P (decl))
@@ -19963,16 +19966,19 @@ decls_for_scope (tree stmt, dw_die_ref context_die,
int depth)
/* Output the DIEs to represent all of the data objects and typedefs
declared directly within this block but not within any nested
sub-blocks. Also, nested function and tag DIEs have been
- generated with a parent of NULL; fix that up now. */
- for (decl = BLOCK_VARS (stmt); decl != NULL; decl = DECL_CHAIN (decl))
-process_scope_var (stmt, decl, NULL_TREE, context_die);
- for (i = 0; i < BLOCK_NUM_NONLOCALIZED_VARS (stmt); i++)
-process_scope_var (stmt, NULL, BLOCK_NONLOCALIZED_VAR (stmt, i),
- context_die);
+ generated with a parent of NULL; fix that up now. We don't
+ have to do this if we're at -g1. */
+ if (debug_info_level > DINFO_LEVEL_TERSE)
+{
+ for (decl = BLOCK_VARS (stmt); decl != NULL; decl = DECL_CHAIN (decl))
+ process_scope_var (stmt, decl, NULL_TREE, context_die);
+ for (i = 0; i < BLOCK_NUM_NONLOCALIZED_VARS (stmt); i++)
+ process_scope_var (stmt, NULL, BLOCK_NONLOCALIZED_VAR (stmt, i),
+ context_die);
+}
- /* If we're at -g1, we're not interested in subblocks. */
- if (debug_info_level <= DINFO_LEVEL_TERSE)
-return;
+ /* Even if we're at -g1, we need to process the subblocks in order to get
+ inlined call information. */
/* Output the DIEs to represent all sub-blocks (and the items declared
therein) of this block. */
@@ -21381,7 +21387,7 @@ dwarf2out_source_line (unsigned int line, const char
*filename,
unsigned int file_num;
dw_line_info_table *table;
- if (debug_info_level < DINFO_LEVEL_NORMAL || line == 0)
+ if (debug_info_level < DINFO_LEVEL_TERSE || line == 0)
return;
/* The discriminator c
Re: [patch] PR 59195: C++ demangler handles conversion operator incorrectly
I've made a small revision to this patch to handle recursive
invocations of d_expression and d_operator_name, restoring the
previous values of is_expression and is_conversion instead of just
setting them to 0 upon return. I've also added the long test case that
results in a substitution misnumbering in the current demangler.
-cary
> 2013-11-19 Cary Coutant
>
> libiberty/
> PR other/59195
> * cp-demangle.c (struct d_info_checkpoint): New struct.
> (struct d_print_info): Add current_template field.
> (d_operator_name): Set flag when processing a conversion
> operator.
> (cplus_demangle_type): When processing for
> a conversion operator, backtrack if necessary.
> (d_expression_1): Renamed from d_expression.
> (d_expression): New wrapper around d_expression_1.
> (d_checkpoint): New function.
> (d_backtrack): New function.
> (d_print_init): Initialize current_template.
> (d_print_comp): Set current_template.
> (d_print_cast): Put current_template in scope for
> printing conversion operator name.
> (cplus_demangle_init_info): Initialize is_expression and
> is_conversion.
> * cp-demangle.h (struct d_info): Add is_expression and
> is_conversion fields.
> * testsuite/demangle-expected: New test cases.
commit 498efd2d720b48641fe0142295f19438601ea2f1
Author: Cary Coutant
Date: Wed Nov 13 09:28:58 2013 -0800
Fix demangler to handle conversion operators correctly.
2013-11-19 Cary Coutant
libiberty/
PR other/59195
* cp-demangle.c (struct d_info_checkpoint): New struct.
(struct d_print_info): Add current_template field.
(d_operator_name): Set flag when processing a conversion
operator.
(cplus_demangle_type): When processing for
a conversion operator, backtrack if necessary.
(d_expression_1): Renamed from d_expression.
(d_expression): New wrapper around d_expression_1.
(d_checkpoint): New function.
(d_backtrack): New function.
(d_print_init): Initialize current_template.
(d_print_comp): Set current_template.
(d_print_cast): Put current_template in scope for
printing conversion operator name.
(cplus_demangle_init_info): Initialize is_expression and
is_conversion.
* cp-demangle.h (struct d_info): Add is_expression and
is_conversion fields.
* testsuite/demangle-expected: New test cases.
diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c
index cbe4d8c..029151e 100644
--- a/libiberty/cp-demangle.c
+++ b/libiberty/cp-demangle.c
@@ -287,6 +287,19 @@ struct d_saved_scope
struct d_print_template *templates;
};
+/* Checkpoint structure to allow backtracking. This holds copies
+ of the fields of struct d_info that need to be restored
+ if a trial parse needs to be backtracked over. */
+
+struct d_info_checkpoint
+{
+ const char *n;
+ int next_comp;
+ int next_sub;
+ int did_subs;
+ int expansion;
+};
+
enum { D_PRINT_BUFFER_LENGTH = 256 };
struct d_print_info
{
@@ -318,6 +331,8 @@ struct d_print_info
struct d_saved_scope *saved_scopes;
/* Number of saved scopes in the above array. */
int num_saved_scopes;
+ /* The nearest enclosing template, if any. */
+ const struct demangle_component *current_template;
};
#ifdef CP_DEMANGLE_DEBUG
@@ -444,6 +459,10 @@ d_add_substitution (struct d_info *, struct
demangle_component *);
static struct demangle_component *d_substitution (struct d_info *, int);
+static void d_checkpoint (struct d_info *, struct d_info_checkpoint *);
+
+static void d_backtrack (struct d_info *, struct d_info_checkpoint *);
+
static void d_growable_string_init (struct d_growable_string *, size_t);
static inline void
@@ -1734,8 +1753,15 @@ d_operator_name (struct d_info *di)
if (c1 == 'v' && IS_DIGIT (c2))
return d_make_extended_operator (di, c2 - '0', d_source_name (di));
else if (c1 == 'c' && c2 == 'v')
-return d_make_comp (di, DEMANGLE_COMPONENT_CAST,
- cplus_demangle_type (di), NULL);
+{
+ struct demangle_component *type;
+ int was_conversion = di->is_conversion;
+
+ di->is_conversion = ! di->is_expression;
+ type = cplus_demangle_type (di);
+ di->is_conversion = was_conversion;
+ return d_make_comp (di, DEMANGLE_COMPONENT_CAST, type, NULL);
+}
else
{
/* LOW is the inclusive lower bound. */
@@ -2284,13 +2310,61 @@ cplus_demangle_type (struct d_info *di)
ret = d_template_param (di);
if (d_peek_char (di) == 'I')
{
- /* This is . The
- part is a substitution
+ /* This
Re: [RFC] Modify -g1 to produce line tables
> Having just looked at the opts.c and tree-ssa-live.c changes, they're fine. Thanks, I've committed the patch. -cary
Re: [RFC] Modify -g1 to produce line tables
>> Let me rebase the >> patch, kick off a bootstrap and regression tests, and I think I can be >> ready to submit it if there are no objections. > > Here's the rebased patch. I'm running the bootstrap and regression tests now. The bootstrap passed with no new regressions. Do I need approval for the changes to tree-ssa-live.c and opts.c? -cary
Re: [google/gcc-4_9] Add gcc driver option -no-pie
>>> If adding a new option, you need to document it in invoke.texi. >> >> Patch updated. > > Is this alright for google/gcc-4_9? +no-pie +Driver RejectNegative Negative(pie) +Create a position dependent executable I'd suggest adding an alias for "-no-pie" (meaning "--no-pie") -- see earlier in common.opt where "-pie" is declared as an alias for "pie", and similarly for "-shared". I wonder about the spelling -- should it be "-no-pie" or "-nopie"? GCC options seem to favor "no" options without a hyphen, but it's not very consistent, so it's probably good the way you've spelled it -- it better matches the way the linker option is spelled (albeit without the "f"). -cary
Re: [google/gcc-4_9] Add gcc driver option -no-pie
>> I'd suggest adding an alias for "-no-pie" (meaning "--no-pie") -- see >> earlier in common.opt where "-pie" is declared as an alias for "pie", >> and similarly for "-shared". > > Patch Updated. OK for google/gcc-4_9 branch. Thanks! -cary
Re: [PATCH 4/n] OpenMP 4.0 offloading infrastructure: lto-wrapper
The linker already has a --strip-lto-sections option, and it's on by
default. I'll approve a patch that modifies gold to recognize
.gnu.offload_lto.* sections as part of --strip-lto-sections.
Really, though, you should be setting the SHF_EXCLUDE bit on these
sections. Do that and no special-casing will be necessary.
Generating a linker script on the fly to discard these sections is, to
me, rather hacky. There are better ways to do it.
-cary
On Thu, Oct 9, 2014 at 11:53 PM, Jakub Jelinek wrote:
> On Fri, Oct 10, 2014 at 12:07:03AM +0400, Ilya Verbin wrote:
>> On 09 Oct 16:07, Ilya Verbin wrote:
>> > > > + /* By default linker does not discard .gnu.offload_lto_*
>> > > > sections. */
>> > > > + const char *linker_script = make_temp_file ("_linker_script.x");
>> > > > + FILE *stream = fopen (linker_script, "w");
>> > > > + if (!stream)
>> > > > + fatal_error ("fopen %s: %m", linker_script);
>> > > > + fprintf (stream, "SECTIONS { /DISCARD/ : { *("
>> > > > + OFFLOAD_SECTION_NAME_PREFIX "*) } }\n");
>> > > > + fclose (stream);
>> > > > + printf ("%s\n", linker_script);
>> > > > +
>> > > > + goto finish;
>> > > > +}
>> > >
>> > > Does this work with gold? Are there any other linkers that support
>> > > plugins,
>> > > but don't support linker scripts this way?
>> >
>> > Oops, gold does not support scripts, outputted from plugins :(
>> > "error: SECTIONS seen after other input files; try -T/--script"
>> >
>> > Probably, we should update default linker scripts in binutils?
>> > But without latest ld/gold all binaries compiled without -flto and with
>> > offloading will contain intermediate bytecode...
>>
>> Actually, this issue is not due to outputting a script from a plugin,
>> gold just does not support partial linker scripts:
>> https://sourceware.org/bugzilla/show_bug.cgi?id=17451
>>
>> So it seems that discarding .gnu.offload_lto_* sections (like it is done for
>> .gnu.lto_*) in the default ld and gold scripts is the right way?
>
> I must say I'm not very much familiar with the linker plugin API, but it
> surprises me that discarding sections is not something it allows.
> Anyway, can you do the partial linker script for the bfd linker (is there
> a way to determine from the linker plugin API if it is gold or bfd ld?), and
> for gold for the time being perhaps strip the sections in lto-wrapper? and
> feed the ET_REL objects with the sections stripped back to the linker
> through the plugin API?
>
> Jakub
Re: [PATCH 4/n] OpenMP 4.0 offloading infrastructure: lto-wrapper
> The question is what will old assemblers and/or linkers do with that, and > if there are any that support linker plugins, but not SHF_EXCLUDE. If it helps answer that question, SHF_EXCLUDE support has been in gold for 6 years, and in gas for 4. -cary
Re: [PATCH 2/3] PR other/61321 - demangler crash on casts in template parameters
Ping. Jason, do you still think the special-case for conversion ops is inappropriate? -cary On Fri, Jul 25, 2014 at 2:16 AM, Pedro Alves wrote: > On 07/24/2014 11:35 PM, Cary Coutant wrote: >>> It seems that the problem here is more general; a template argument list is >>> not in scope within that same template argument list. Can't we fix that >>> without special-casing conversion ops? >> >> I think conversion ops really are a special case. > > Thanks Cary. FWIW, I agree. > > (GDB 7.8 hasn't been released yet, though it's close. If this > patch is approved as is, we'll be able to have the crash > fixed there. If this requires a significant rewrite though, > I'm afraid I might not be able to do it myself anytime soon.) > >> It's the only case >> where the template parameters refer to the template argument list from >> the cast operator's enclosing template. In a cast expression, like >> anywhere else you might have a template parameter, the template >> parameter refers to the template argument list of the immediately >> enclosing template. >> >> I think this note from Section 5.1.3 (Operator Encodings) of the ABI >> is what makes this a special case (it's an informative comment in the >> document, but seems to me to be normative): >> >> "For a user-defined conversion operator the result type (i.e., the >> type to which the operator converts) is part of the mangled name of >> the function. If the conversion operator is a member template, the >> result type will appear before the template parameters. There may be >> forward references in the result type to the template parameters." >> > > -- > Thanks, > Pedro Alves >
Re: [PATCH 4/n] OpenMP 4.0 offloading infrastructure: lto-wrapper
> My preference would be to add the | SECTION_EXCLUDE unconditionally, and > instead guard the > if (flags & SECTION_EXCLUDE) > *f++ = 'e'; > in varasm.c (default_elf_asm_named_section). The only other user of > SECTION_EXCLUDE seems to be -gsplit-dwarf right now, Cary, is such a change > ok with you? Yes, that sounds fine. > If you have new gas and old linker, I'd expect it would just ignore > SHF_EXCLUDE. Agreed. -cary
[PATCH] Add top-level config support for gold mips target
This patch adds support for the mips target in gold.
OK to commit?
-cary
2014-10-20 Cary Coutant
* configure (--enable-gold): Add mips*-*-*.
* configure.ac: Regenerate.
Index: configure
===
--- configure (revision 216487)
+++ configure (working copy)
@@ -2941,7 +2941,7 @@ case "${ENABLE_GOLD}" in
# Check for target supported by gold.
case "${target}" in
i?86-*-* | x86_64-*-* | sparc*-*-* | powerpc*-*-* | arm*-*-* \
-| aarch64*-*-* | tilegx*-*-*)
+| aarch64*-*-* | tilegx*-*-* | mips*-*-*)
configdirs="$configdirs gold"
if test x${ENABLE_GOLD} = xdefault; then
default_ld=gold
Index: configure.ac
===
--- configure.ac(revision 216487)
+++ configure.ac(working copy)
@@ -332,7 +332,7 @@ case "${ENABLE_GOLD}" in
# Check for target supported by gold.
case "${target}" in
i?86-*-* | x86_64-*-* | sparc*-*-* | powerpc*-*-* | arm*-*-* \
-| aarch64*-*-* | tilegx*-*-*)
+| aarch64*-*-* | tilegx*-*-* | mips*-*-*)
configdirs="$configdirs gold"
if test x${ENABLE_GOLD} = xdefault; then
default_ld=gold
[google/gcc-4_8] Fix ICE with -gsplit-dwarf and FDO
Fix ICE when -gsplit-dwarf is used with -freorder-blocks-and-partition.
When FDO and -freorder-blocks-and-partition are enabled, it's possible
that by the time we get to optimize_location_lists, we have not yet
created any .debug_addr table entries. If -gsplit-dwarf is also enabled,
we still need to assign .debug_addr indexes to the location lists, so
we should be calling index_location_lists even if addr_index_table
is still NULL.
This patch is for the google/gcc-4_8 branch. I will checkin the fix
to trunk and the gcc-4_9 branch once I have a test case to exercise the
failure.
Google ref: b/15417905
2014-06-04 Cary Coutant
gcc/
* dwarf2out.c (dwarf2out_finish): Call index_location_lists
even if addr_index_table is NULL.
Index: gcc/dwarf2out.c
===
--- gcc/dwarf2out.c (revision 211246)
+++ gcc/dwarf2out.c (working copy)
@@ -24297,18 +24297,23 @@ dwarf2out_finish (const char *filename)
dwarf_strict ? DW_AT_macro_info : DW_AT_GNU_macros,
macinfo_section_label);
- if (dwarf_split_debug_info && addr_index_table != NULL)
+ if (dwarf_split_debug_info)
{
/* optimize_location_lists calculates the size of the lists,
so index them first, and assign indices to the entries.
Although optimize_location_lists will remove entries from
the table, it only does so for duplicates, and therefore
only reduces ref_counts to 1. */
- unsigned int index = 0;
index_location_lists (comp_unit_die ());
- htab_traverse_noresize (addr_index_table,
- index_addr_table_entry, &index);
+
+ if (addr_index_table != NULL)
+{
+ unsigned int index = 0;
+ htab_traverse_noresize (addr_index_table,
+ index_addr_table_entry, &index);
+}
}
+
if (have_location_lists)
optimize_location_lists (comp_unit_die ());
Re: [GOOGLE] Emit linkage_name when built with -gmlt and for abstract decls
> This will increase c++ g1/g2 binary size a little. For all spec > cint2006 benchmarks, the binary size change is shown below. > > 400 0.00% 0.00% 0.00% 0.00% > 401 0.00% 0.00% 0.00% 0.00% > 403 0.00% 0.00% 0.00% 0.00% > 429 0.00% 0.00% 0.00% 0.00% > 445 0.00% 0.00% 0.00% 0.00% > 456 0.00% 0.00% 0.00% 0.00% > 458 0.00% 0.00% 0.00% 0.00% > 462 0.00% 0.00% 0.00% 0.00% > 464 0.00% 0.00% 0.00% 0.00% > 471 1.28% 0.20% 1.23% 0.15% > 473 0.36% 0.00% 0.35% 0.01% > 483 12.79% 1.73% 13.65% 2.12% > geomean 1.14% 0.16% 1.20% 0.19% > > The 4 columns are: > > o0 -g1 > o0 -g2 > o2 -g1 > o2 -g2 We expect this to affect C++ code, so only the last three of those benchmarks are really meaningful -- if you omit the C benchmarks, the geomean will be a bit higher. Why, I wonder, is 483 affected so much more than 471 and 473? At any rate, -g2 doesn't seem to be affected too much. I wish the -g1 numbers for 483 weren't quite so high, but I understand the importance for FDO, and there isn't a lot of current usage of -g1, so it's OK with me for trunk. I hope we can fine-tune this a bit in the future, though. -cary
[patch] Fix ICEs with -gsplit-dwarf
This patch fixes a couple of ICEs when using -gsplit-dwarf.
When compiling a small-enough compilation unit that has no address table
entries, but complex enough that -freorder-blocks-and-partition produces
location lists, dwarf2out_finish does not call index_location_lists, but
optimize_location_lists will later assume that the addr_index_table has
been indexed.
Google ref: b/15417905
When resolve_addr_in_expr replaces a CONST_STRING rtx, it directly
updates the pointer to the old expression with the new one. In the
case of a DW_OP_GNU_addr_index or DW_OP_GNU_const_index, that pointer
may be in an address table entry, which is keyed by the rtx. Instead
of directly replacing the pointer, we need to remove the old address
table entry (i.e., decrement its reference count), and add a new one.
Google ref: b/15957101
Bootstrapped with no new regressions, and committed as r212211.
-cary
2014-07-01 Cary Coutant
gcc/
* dwarf2out.c (remove_addr_table_entry): Remove unnecessary hash table
lookup.
(resolve_addr_in_expr): When replacing the rtx in a location list
entry, get a new address table entry.
(dwarf2out_finish): Call index_location_lists even if there are no
addr_index_table entries yet.
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 8caf940..94e98a1 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -4222,13 +4222,10 @@ add_addr_table_entry (void *addr, enum ate_kind kind)
static void
remove_addr_table_entry (addr_table_entry *entry)
{
- addr_table_entry *node;
-
gcc_assert (dwarf_split_debug_info && addr_index_table);
- node = (addr_table_entry *) htab_find (addr_index_table, entry);
/* After an index is assigned, the table is frozen. */
- gcc_assert (node->refcount > 0 && node->index == NO_INDEX_ASSIGNED);
- node->refcount--;
+ gcc_assert (entry->refcount > 0 && entry->index == NO_INDEX_ASSIGNED);
+ entry->refcount--;
}
/* Given a location list, remove all addresses it refers to from the
@@ -23102,11 +23099,16 @@ resolve_addr_in_expr (dw_loc_descr_ref loc)
break;
case DW_OP_GNU_addr_index:
case DW_OP_GNU_const_index:
- if ((loc->dw_loc_opc == DW_OP_GNU_addr_index
-|| (loc->dw_loc_opc == DW_OP_GNU_const_index && loc->dtprel))
- && resolve_one_addr (&loc->dw_loc_oprnd1.val_entry->addr.rtl,
-NULL))
- return false;
+ if (loc->dw_loc_opc == DW_OP_GNU_addr_index
+|| (loc->dw_loc_opc == DW_OP_GNU_const_index && loc->dtprel))
+ {
+rtx rtl = loc->dw_loc_oprnd1.val_entry->addr.rtl;
+if (resolve_one_addr (&rtl, NULL))
+ return false;
+remove_addr_table_entry (loc->dw_loc_oprnd1.val_entry);
+loc->dw_loc_oprnd1.val_entry =
+add_addr_table_entry (rtl, ate_kind_rtx);
+ }
break;
case DW_OP_const4u:
case DW_OP_const8u:
@@ -24198,18 +24200,23 @@ dwarf2out_finish (const char *filename)
dwarf_strict ? DW_AT_macro_info : DW_AT_GNU_macros,
macinfo_section_label);
- if (dwarf_split_debug_info && addr_index_table != NULL)
+ if (dwarf_split_debug_info)
{
/* optimize_location_lists calculates the size of the lists,
so index them first, and assign indices to the entries.
Although optimize_location_lists will remove entries from
the table, it only does so for duplicates, and therefore
only reduces ref_counts to 1. */
- unsigned int index = 0;
index_location_lists (comp_unit_die ());
- htab_traverse_noresize (addr_index_table,
- index_addr_table_entry, &index);
+
+ if (addr_index_table != NULL)
+{
+ unsigned int index = 0;
+ htab_traverse_noresize (addr_index_table,
+ index_addr_table_entry, &index);
+}
}
+
if (have_location_lists)
optimize_location_lists (comp_unit_die ());
Re: [patch] Fix ICEs with -gsplit-dwarf
Any objections to backporting these fixes to the 4.9 branch?
-cary
On Tue, Jul 1, 2014 at 2:37 PM, Cary Coutant wrote:
> This patch fixes a couple of ICEs when using -gsplit-dwarf.
>
> When compiling a small-enough compilation unit that has no address table
> entries, but complex enough that -freorder-blocks-and-partition produces
> location lists, dwarf2out_finish does not call index_location_lists, but
> optimize_location_lists will later assume that the addr_index_table has
> been indexed.
> Google ref: b/15417905
>
> When resolve_addr_in_expr replaces a CONST_STRING rtx, it directly
> updates the pointer to the old expression with the new one. In the
> case of a DW_OP_GNU_addr_index or DW_OP_GNU_const_index, that pointer
> may be in an address table entry, which is keyed by the rtx. Instead
> of directly replacing the pointer, we need to remove the old address
> table entry (i.e., decrement its reference count), and add a new one.
> Google ref: b/15957101
>
> Bootstrapped with no new regressions, and committed as r212211.
>
> -cary
>
>
> 2014-07-01 Cary Coutant
>
> gcc/
> * dwarf2out.c (remove_addr_table_entry): Remove unnecessary hash table
> lookup.
> (resolve_addr_in_expr): When replacing the rtx in a location list
> entry, get a new address table entry.
> (dwarf2out_finish): Call index_location_lists even if there are no
> addr_index_table entries yet.
>
>
> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> index 8caf940..94e98a1 100644
> --- a/gcc/dwarf2out.c
> +++ b/gcc/dwarf2out.c
> @@ -4222,13 +4222,10 @@ add_addr_table_entry (void *addr, enum ate_kind kind)
> static void
> remove_addr_table_entry (addr_table_entry *entry)
> {
> - addr_table_entry *node;
> -
>gcc_assert (dwarf_split_debug_info && addr_index_table);
> - node = (addr_table_entry *) htab_find (addr_index_table, entry);
>/* After an index is assigned, the table is frozen. */
> - gcc_assert (node->refcount > 0 && node->index == NO_INDEX_ASSIGNED);
> - node->refcount--;
> + gcc_assert (entry->refcount > 0 && entry->index == NO_INDEX_ASSIGNED);
> + entry->refcount--;
> }
>
> /* Given a location list, remove all addresses it refers to from the
> @@ -23102,11 +23099,16 @@ resolve_addr_in_expr (dw_loc_descr_ref loc)
> break;
>case DW_OP_GNU_addr_index:
>case DW_OP_GNU_const_index:
> - if ((loc->dw_loc_opc == DW_OP_GNU_addr_index
> -|| (loc->dw_loc_opc == DW_OP_GNU_const_index && loc->dtprel))
> - && resolve_one_addr (&loc->dw_loc_oprnd1.val_entry->addr.rtl,
> -NULL))
> - return false;
> + if (loc->dw_loc_opc == DW_OP_GNU_addr_index
> +|| (loc->dw_loc_opc == DW_OP_GNU_const_index && loc->dtprel))
> + {
> +rtx rtl = loc->dw_loc_oprnd1.val_entry->addr.rtl;
> +if (resolve_one_addr (&rtl, NULL))
> + return false;
> +remove_addr_table_entry (loc->dw_loc_oprnd1.val_entry);
> +loc->dw_loc_oprnd1.val_entry =
> +add_addr_table_entry (rtl, ate_kind_rtx);
> + }
> break;
>case DW_OP_const4u:
>case DW_OP_const8u:
> @@ -24198,18 +24200,23 @@ dwarf2out_finish (const char *filename)
>dwarf_strict ? DW_AT_macro_info : DW_AT_GNU_macros,
>macinfo_section_label);
>
> - if (dwarf_split_debug_info && addr_index_table != NULL)
> + if (dwarf_split_debug_info)
> {
>/* optimize_location_lists calculates the size of the lists,
> so index them first, and assign indices to the entries.
> Although optimize_location_lists will remove entries from
> the table, it only does so for duplicates, and therefore
> only reduces ref_counts to 1. */
> - unsigned int index = 0;
>index_location_lists (comp_unit_die ());
> - htab_traverse_noresize (addr_index_table,
> - index_addr_table_entry, &index);
> +
> + if (addr_index_table != NULL)
> +{
> + unsigned int index = 0;
> + htab_traverse_noresize (addr_index_table,
> + index_addr_table_entry, &index);
> +}
> }
> +
>if (have_location_lists)
> optimize_location_lists (comp_unit_die ());
>
Re: [PATCH] DWARF add DW_AT_noreturn on noreturn function subprogram.
> Presumably we don't have any sense when the values will be assigned, right? > Any chance we could speed that along a bit? As Jason said, the value in the current draft is unlikely to change, and I think he and I can probably lobby to keep it unchanged if there any danger that the numbering will change. The committee doesn't really like it when we jump the gun and use values before the final spec is published, but as a practical matter, it's often necessary and (at this stage) pretty safe. -cary
Re: [PATCH] DWARF add DW_AT_noreturn on noreturn function subprogram.
[+cc Michael Eager] > Rather than having to lobby to keep it unchanged because we jumped the gun, > can we lobby to get the number assigned in the near future rather than in > the potentially far future? That feels more cooperative to me :-) > > Would that make Michael happier? I'm pretty confident that Michael will say, "don't rely on the value until the final spec is published." (He's told me exactly that in the past. I've been guilty of jumping the gun on a couple of DW_LANG codes and a few DW_AT values.) But we should let Michael answer for himself... Michael, for your reference, here's the start of this thread: https://gcc.gnu.org/ml/gcc-patches/2014-11/msg03195.html and its continuation into December: https://gcc.gnu.org/ml/gcc-patches/2014-12/msg00099.html Michael, are you OK with sharing your target dates for publishing the spec? -cary
Re: [PATCH] Add top-level config support for gold mips target
Ping^2.
-cary
On Wed, Oct 29, 2014 at 12:04 PM, Cary Coutant wrote:
> Ping?
>
> On Mon, Oct 20, 2014 at 10:31 AM, Cary Coutant wrote:
>> This patch adds support for the mips target in gold.
>>
>> OK to commit?
>>
>> -cary
>>
>>
>> 2014-10-20 Cary Coutant
>>
>> * configure (--enable-gold): Add mips*-*-*.
>> * configure.ac: Regenerate.
>>
>>
>> Index: configure
>> ===
>> --- configure (revision 216487)
>> +++ configure (working copy)
>> @@ -2941,7 +2941,7 @@ case "${ENABLE_GOLD}" in
>># Check for target supported by gold.
>>case "${target}" in
>> i?86-*-* | x86_64-*-* | sparc*-*-* | powerpc*-*-* | arm*-*-* \
>> -| aarch64*-*-* | tilegx*-*-*)
>> +| aarch64*-*-* | tilegx*-*-* | mips*-*-*)
>> configdirs="$configdirs gold"
>> if test x${ENABLE_GOLD} = xdefault; then
>> default_ld=gold
>> Index: configure.ac
>> ===
>> --- configure.ac(revision 216487)
>> +++ configure.ac(working copy)
>> @@ -332,7 +332,7 @@ case "${ENABLE_GOLD}" in
>># Check for target supported by gold.
>>case "${target}" in
>> i?86-*-* | x86_64-*-* | sparc*-*-* | powerpc*-*-* | arm*-*-* \
>> -| aarch64*-*-* | tilegx*-*-*)
>> +| aarch64*-*-* | tilegx*-*-* | mips*-*-*)
>> configdirs="$configdirs gold"
>> if test x${ENABLE_GOLD} = xdefault; then
>> default_ld=gold
Re: [google/gcc-4_9] Minor changes to -ftwo-level-line-tables
>> @@ -21817,22 +21823,39 @@ out_subprog_directive (subprog_entry *su
>> {
>>tree decl = subprog->decl;
>>tree decl_name = DECL_NAME (decl);
>> - const char *name;
>> + tree origin;
>
> Explicitly initialize origin to NULL_TREE;
Done.
>> + /* For inlined subroutines, use the linkage name.
>> + If -ftwo-level-all-subprogs is set, use the linkage name
>> + for all subroutines. */
>> + if (subprog->is_inlined || flag_two_level_all_subprogs)
>> {
>> - name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl));
>> - if (name[0] == '*')
>> -name++;
>> + if (DECL_ASSEMBLER_NAME (origin))
>> + {
>> + name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (origin));
>> + if (name[0] == '*')
>> + name++;
>> + }
>> + else
>> + name = dwarf2_name (origin, 0);
>> }
>>else
>> -name = dwarf2_name (decl, 0);
>> +{
>> + /* To save space, we don't emit the name for non-inlined
>> + subroutines, whose linkage names are available from the
>> + object file's symbol table. */
>
> flag_two_level_all_subprogs will be 1 by default. This mean "else"
> branch is not the default behavior?
No, I changed the default in common.opt:
ftwo-level-all-subprogs
-Common Report Var(flag_two_level_all_subprogs) Init(1)
+Common Report Var(flag_two_level_all_subprogs) Init(0)
When generating two-level line tables in DWARF (experimental),
-generate subprogram table entries for all functions.
+add linkage names for all functions (not just inlined functions).
-cary
[patch] Update my email address
I'm retiring, and my last day at google is this Friday, April 10. I plan to continue to contribute to GCC and binutils in my retirement. I've updated the MAINTAINERS file to use my personal address, ccout...@gmail.com. -cary 2015-04-08 Cary Coutant * MAINTAINERS: Update my email address. Index: MAINTAINERS === --- MAINTAINERS (revision 221926) +++ MAINTAINERS (working copy) @@ -195,7 +195,7 @@ caller-save.c Jeff Law debugging code Jim Wilson dwarf debugging code Jason Merrill -dwarf debugging code Cary Coutant +dwarf debugging code Cary Coutant c++ runtime libs Paolo Carlini c++ runtime libs Ulrich Drepper c++ runtime libs Benjamin De Kosnik @@ -300,7 +300,7 @@ libsanitizer, asan.cDmitry Vyukov loop optimizer Daniel Berlin LTORichard Biener -LTO plugin Cary Coutant +LTO plugin Cary Coutant Plugin Le-Chun Wu register allocationPeter Bergner register allocationKenneth Zadeck @@ -365,7 +365,7 @@ William Cohen Josh Conner R. Kelley Cook Christian Cornelssen -Cary Coutant +Cary Coutant Lawrence Crowl Ian Dall David Daney
Re: PR lto/64837: lto plugin doesn't call ld_plugin_release_input_file
>>>This patch makes claim_file_handler to call release_input_file after it >>>finishes processing input file. OK for trunk? >> >> OK. How did you test this? > > I did normal bootstrap and "make check" on Linux/x86-64. > I also run ld.bfd and ld.gold by hand to verify that release_input_file > is called. But release_input_file is only supposed to be used after calling get_input_file from the all_symbols_read handler. It's not needed in the claim_file handler. If gold isn't freeing the file descriptor properly in that case, it's a bug entirely within gold (I think). I'm looking at it. -cary
Re: [google/gcc-4_9] Add -ftwo-level-line-tables and -gline-tables-only options
>> +static subprog_entry *
>> +add_subprog_entry (tree decl, bool is_inlined)
>> +{
>> + subprog_entry **slot;
>> + subprog_entry *entry;
>> +
>> + slot = subprog_table->find_slot_with_hash (decl, DECL_UID (decl), INSERT);
>> + if (*slot == HTAB_EMPTY_ENTRY)
>> +{
>> + entry = XCNEW (struct subprog_entry);
>> + entry->decl = decl;
>> + entry->is_inlined = is_inlined;
>> + entry->subprog_num = 0;
>> + *slot = entry;
>> +}
>> + else if (is_inlined)
>
> When will the logic go into else branch?
When we already have an entry for the given subprogram (e.g., the same
subroutine gets inlined in multiple places).
>> +/* For two-level line tables, a map from block number to an
>> + inlined call chain. */
>> +
>> +struct block_entry
>> +{
>> + unsigned int block_num;
>> + struct subprog_entry *subprog;
>> + struct block_entry *caller;
>> + location_t caller_loc;
>> +};
>> +
>> +struct block_hasher : typed_free_remove
>> +{
>> + typedef block_entry value_type;
>> + typedef unsigned int compare_type;
>> + static hashval_t hash (const value_type *);
>> + static bool equal (const value_type *, const compare_type *);
>> +};
>> +
>> +inline hashval_t
>> +block_hasher::hash (const value_type *p)
>> +{
>> + return (hashval_t) p->block_num;
>> +}
>> +
>> +inline bool
>> +block_hasher::equal (const value_type *p1, const compare_type *p2)
>> +{
>> + return p1->block_num == *p2;
>> +}
>> +
>> +static hash_table *block_table;
>
> Not quite clear why we need block_table. This table is not gonna be
> emitted. And we can easily get subprog_entry through block->block_num
When final_scan_insn() calls dwarf2out_begin_block(), all it passes is
a block number. I don't know a way to get from block number to the
block, so I traverse all the blocks of a function when
dwarf2out_begin_function() is called, and build this table. Now in
dwarf2out_source_line, I can look at the current block number and tell
what the inline call stack is.
>> +#ifdef DEBUG_TWO_LEVEL
>> + static unsigned int level = 0;
>> +#endif
>> +
>> + if (block == NULL)
>> +return;
>> +
>> +#ifdef DEBUG_TWO_LEVEL
>
> Shall this be controlled by dump options with TDF_DETAILS dump_flag?
I don't see the need -- I'll rip this out before submitting for trunk.
I'd have ripped it out already, but thought it might be useful for a
little while longer.
>> + block_num = BLOCK_NUMBER (block);
>> + slot = block_table->find_slot_with_hash (&block_num, (hashval_t)
>> block_num, INSERT);
>> + if (*slot != HTAB_EMPTY_ENTRY)
>
> Instead of return, can you assert that the data stored in *slot is
> consistent with the new data? Or should *slot never be
> HTAB_EMPTY_ENTRY?
It should probably be consistent, but I wasn't absolutely sure, and I
didn't want to have the compiler crash when either version of the data
is probably good enough. It might not be empty, because I might have
already added the block number to the table in the loop over the
parent node's BLOCK_FRAGMENT_CHAIN. (I may not need to have that loop
at all, if it's always the case that the blocks in
BLOCK_FRAGMENT_CHAIN are also contained in the subtree under
BLOCK_SUBBLOCKS. I'm being conservative here.)
-cary
Re: [google/gcc-4_9] Add -ftwo-level-line-tables and -gline-tables-only options
>> > Not quite clear why we need block_table. This table is not gonna be >> > emitted. And we can easily get subprog_entry through block->block_num >> >> When final_scan_insn() calls dwarf2out_begin_block(), all it passes is >> a block number. I don't know a way to get from block number to the >> block, so I traverse all the blocks of a function when >> dwarf2out_begin_function() is called, and build this table. Now in >> dwarf2out_source_line, I can look at the current block number and tell >> what the inline call stack is. > > Is it correct that block_num has 1-1 mapping with block_table. And > block_table has 1-1 mapping with logical_table? The first part, yes -- there's one entry in block_table for each block_num in the function tree. But two or more blocks may map to a single logical, and some blocks may not correspond to a logical at all -- if dwarf2out_source_line() is never called for a block, I'll never create a logical for it. -cary
Re: [google/gcc-4_9] Add -ftwo-level-line-tables and -gline-tables-only options
>>> Is it correct that block_num has 1-1 mapping with block_table. And >>> block_table has 1-1 mapping with logical_table? >> >> The first part, yes -- there's one entry in block_table for each >> block_num in the function tree. But two or more blocks may map to a >> single logical, and some blocks may not correspond to a logical at all >> -- if dwarf2out_source_line() is never called for a block, I'll never >> create a logical for it. > > I don't understand why multiple blocks may map to a single > logical_entry. Can you give an example? Sorry, you may be right that two different block cannot map to a single logical entry. But I don't think that's relevant. I create a logical for a specific combination of file/line/discriminator/subprog/context, which I obtain from the block entry. It is definitely possible for many logicals to map to a single block, though, so there is not a 1-1 mapping. -cary
Re: [google/gcc-4_9] Add -ftwo-level-line-tables and -gline-tables-only options
Here's a very slightly revised patch, fixing a couple of bugs found
during GDB testing.
In out_logical_entry, I should pass along the value of is_stmt when
creating a logical for the calling context, so that we get a
breakpoint location for the point of call:
context = out_logical_entry (table, caller_file_num, s.line,
caller_discrim, block->caller,
+ is_stmt, true);
And later in out_logical_entry, I should set table->is_stmt only when
we explicitly set is_stmt in the assembly output:
if (is_stmt != table->is_stmt)
{
fputs (" is_stmt ", asm_out_file);
putc (is_stmt ? '1' : '0', asm_out_file);
+ table->is_stmt = is_stmt;
}
Instead of at the bottom of the function:
table->file_num = file_num;
table->line_num = line_num;
table->discrim_num = discriminator;
- table->is_stmt = is_stmt;
table->in_use = true;
This sometimes caused lines where is_stmt should have been set to be
marked is_stmt == 0 because we thought it was already set.
-cary
patch-two-level-2
Description: Binary data
Re: [PATCH] Added PLUGIN_FINISH_TYPE callback on enum type processing
> I am forwarding this reply to Cary Coutant, Diego Novillo and Le-Chun > Wu, as they were listed as the plugin maintainers. > > Cary, Diego, Le-Chun, please let me know if you are on it, or if I > should send it to someone else. Sorry, this isn't my kind of plugin -- I'm a maintainer for the LTO linker plugin, but this looks like it's related to GCC plugins. Diego or Le-Chun should be able to help, though. -cary
Re: PR lto/64837: lto plugin doesn't call ld_plugin_release_input_file
The plugin is not supposed to call release_input_file from the claim_file handler. That interface is only for releasing a file descriptor obtained via get_input_file during the all_symbols_read callback. When the linker calls the claim_file handler, the file descriptor is open, and the plugin is required to leave it open; the linker manages the file descriptor at that point. The get_input_file/release_input_file pair of interfaces was added later, for the benefit of another (non-LTO) plugin (although I think the LLVM LTO plugin does use that pair during the all_symbols_read callback). This is described here: https://gcc.gnu.org/wiki/whopr/driver If you're going to insist on calling the release_input_file API from the claim_file handler, I'm going to have to fix gold to ignore the call to avoid a premature unlock of the object file. -cary On Wed, Jan 28, 2015 at 4:02 PM, H.J. Lu wrote: > On Wed, Jan 28, 2015 at 11:37 AM, H.J. Lu wrote: >> On Wed, Jan 28, 2015 at 11:19 AM, Richard Biener >> wrote: >>> On January 28, 2015 7:12:43 PM CET, "H.J. Lu" wrote: Hi, This patch makes claim_file_handler to call release_input_file after it finishes processing input file. OK for trunk? >>> >>> OK. How did you test this? >> >> I did normal bootstrap and "make check" on Linux/x86-64. >> I also run ld.bfd and ld.gold by hand to verify that release_input_file >> is called. >> > > This is needed for LTO build. ar/nm/ranlib don't provide > release_input_file. I checked it in as an obvious fix. > > -- > H.J. > --- > Index: ChangeLog > === > --- ChangeLog (revision 220212) > +++ ChangeLog (working copy) > @@ -1,5 +1,10 @@ > 2015-01-28 H.J. Lu > > + * lto-plugin.c (claim_file_handler): Call release_input_file only > + if it is not NULL. > + > +2015-01-28 H.J. Lu > + > PR lto/64837 > * lto-plugin.c (release_input_file): New. > (claim_file_handler): Call release_input_file. > Index: lto-plugin.c > === > --- lto-plugin.c (revision 220212) > +++ lto-plugin.c (working copy) > @@ -1007,7 +1007,8 @@ claim_file_handler (const struct ld_plug >if (obj.objfile) > simple_object_release_read (obj.objfile); > > - release_input_file (file); > + if (release_input_file) > +release_input_file (file); > >return LDPS_OK; > }
Re: [PATCH] Add top-level config support for gold mips target
Ping^3. Should I be addressing this to someone else?
-cary
On Mon, Dec 1, 2014 at 2:15 PM, Cary Coutant wrote:
> Ping^2.
>
> -cary
>
> On Wed, Oct 29, 2014 at 12:04 PM, Cary Coutant wrote:
>> Ping?
>>
>> On Mon, Oct 20, 2014 at 10:31 AM, Cary Coutant wrote:
>>> This patch adds support for the mips target in gold.
>>>
>>> OK to commit?
>>>
>>> -cary
>>>
>>>
>>> 2014-10-20 Cary Coutant
>>>
>>> * configure (--enable-gold): Add mips*-*-*.
>>> * configure.ac: Regenerate.
>>>
>>>
>>> Index: configure
>>> ===
>>> --- configure (revision 216487)
>>> +++ configure (working copy)
>>> @@ -2941,7 +2941,7 @@ case "${ENABLE_GOLD}" in
>>># Check for target supported by gold.
>>>case "${target}" in
>>> i?86-*-* | x86_64-*-* | sparc*-*-* | powerpc*-*-* | arm*-*-* \
>>> -| aarch64*-*-* | tilegx*-*-*)
>>> +| aarch64*-*-* | tilegx*-*-* | mips*-*-*)
>>> configdirs="$configdirs gold"
>>> if test x${ENABLE_GOLD} = xdefault; then
>>> default_ld=gold
>>> Index: configure.ac
>>> ===
>>> --- configure.ac(revision 216487)
>>> +++ configure.ac(working copy)
>>> @@ -332,7 +332,7 @@ case "${ENABLE_GOLD}" in
>>># Check for target supported by gold.
>>>case "${target}" in
>>> i?86-*-* | x86_64-*-* | sparc*-*-* | powerpc*-*-* | arm*-*-* \
>>> -| aarch64*-*-* | tilegx*-*-*)
>>> +| aarch64*-*-* | tilegx*-*-* | mips*-*-*)
>>> configdirs="$configdirs gold"
>>> if test x${ENABLE_GOLD} = xdefault; then
>>> default_ld=gold
Re: PR lto/64837: lto plugin doesn't call ld_plugin_release_input_file
>>If you're going to insist on calling the release_input_file API from >>the claim_file handler, I'm going to have to fix gold to ignore the >>call to avoid a premature unlock of the object file. > > What's the proper solution for not leaking those filedescriptors? There was a bug in gold where it wasn't unlocking external members of thin archives that got claimed by the plugin. See PR 15660: https://sourceware.org/bugzilla/show_bug.cgi?id=15660 -cary
Re: [RFC PATCH] Emit DW_LANG_Fortran{03,08}
> DW_LANG_Fortran03 and DW_LANG_Fortran08 DW_AT_language values were recently > accepted into DWARF5. This patch changes GCC to handle those similarly to > how e.g. the -std=c++11, -std=c++14 or -std=c11 are handled. > > As it will take some time for consumers to catch up, I'm enabling that > only if -gdwarf-5 is used for now. My concern with enabling -gdwarf-5 at this point is that all we're really doing with it is enabling a subset of DWARF-5 features (as we did with -gdwarf-4). We're still putting a version number of 2 in the compilation unit header! But I guess even upgrading the CU header to version 3 is something not all consumers are yet ready for. As long as we selectively enable DWARF-5 features while still claiming to be DWARF-2, I guess we're OK, but how will we decide to upgrade fully beyond DWARF-2, and what option will we use for that? The DWARF bits of this patch are OK with me. -cary
Re: [RFC PATCH] Emit DW_LANG_Fortran{03,08}
> PS: Talking about DWARF5, do you know when it will be available as public > draft? I am especially looking forward to > http://dwarfstd.org/ShowIssue.php?issue=121221.1 (Allow DW_AT_type with > DW_TAG_string_type), which would be a low-hanging fruit in terms of > implementation. Contrary to the array additions of 130313.5. It should be available for public review in 3-4 months. We need to do a thorough review of the draft document, and tidy up a few loose ends, but it's pretty much done. I see no reason why you couldn't go ahead and implement what's in 121221.1 as an extension under a (!dwarf_strict) guard. Unless it would really confuse old debuggers, I don't think you'd need to guard it with -gdwarf-5. -cary
Re: [RFC PATCH] Emit DW_LANG_Fortran{03,08}
>> did with -gdwarf-4). We're still putting a version number of 2 in the >> compilation unit header! But I guess even upgrading the CU header to > > We are not. On most targets we default to -gdwarf-4 and emit v. 4: Oops, sorry, you're right. I carelessly misread this: dw2_asm_output_data (2, ver, "DWARF version number"); and read the "2" as the version, not the size. As long as we're capping the version number at 4 until we can actually write a proper version 5 header, this is fine. -cary
Re: [PATCH] Add top-level config support for gold mips target
Ping?
On Mon, Oct 20, 2014 at 10:31 AM, Cary Coutant wrote:
> This patch adds support for the mips target in gold.
>
> OK to commit?
>
> -cary
>
>
> 2014-10-20 Cary Coutant
>
> * configure (--enable-gold): Add mips*-*-*.
> * configure.ac: Regenerate.
>
>
> Index: configure
> ===
> --- configure (revision 216487)
> +++ configure (working copy)
> @@ -2941,7 +2941,7 @@ case "${ENABLE_GOLD}" in
># Check for target supported by gold.
>case "${target}" in
> i?86-*-* | x86_64-*-* | sparc*-*-* | powerpc*-*-* | arm*-*-* \
> -| aarch64*-*-* | tilegx*-*-*)
> +| aarch64*-*-* | tilegx*-*-* | mips*-*-*)
> configdirs="$configdirs gold"
> if test x${ENABLE_GOLD} = xdefault; then
> default_ld=gold
> Index: configure.ac
> ===
> --- configure.ac(revision 216487)
> +++ configure.ac(working copy)
> @@ -332,7 +332,7 @@ case "${ENABLE_GOLD}" in
># Check for target supported by gold.
>case "${target}" in
> i?86-*-* | x86_64-*-* | sparc*-*-* | powerpc*-*-* | arm*-*-* \
> -| aarch64*-*-* | tilegx*-*-*)
> +| aarch64*-*-* | tilegx*-*-* | mips*-*-*)
> configdirs="$configdirs gold"
> if test x${ENABLE_GOLD} = xdefault; then
> default_ld=gold
Re: [PATCH 2/3] PR other/61321 - demangler crash on casts in template parameters
Ping. I'm getting more reports of this bug internally, and it would be nice to have the fix upstream. -cary On Mon, Oct 13, 2014 at 11:43 AM, Cary Coutant wrote: > Ping. Jason, do you still think the special-case for conversion ops is > inappropriate? > > -cary > > > On Fri, Jul 25, 2014 at 2:16 AM, Pedro Alves wrote: >> On 07/24/2014 11:35 PM, Cary Coutant wrote: >>>> It seems that the problem here is more general; a template argument list is >>>> not in scope within that same template argument list. Can't we fix that >>>> without special-casing conversion ops? >>> >>> I think conversion ops really are a special case. >> >> Thanks Cary. FWIW, I agree. >> >> (GDB 7.8 hasn't been released yet, though it's close. If this >> patch is approved as is, we'll be able to have the crash >> fixed there. If this requires a significant rewrite though, >> I'm afraid I might not be able to do it myself anytime soon.) >> >>> It's the only case >>> where the template parameters refer to the template argument list from >>> the cast operator's enclosing template. In a cast expression, like >>> anywhere else you might have a template parameter, the template >>> parameter refers to the template argument list of the immediately >>> enclosing template. >>> >>> I think this note from Section 5.1.3 (Operator Encodings) of the ABI >>> is what makes this a special case (it's an informative comment in the >>> document, but seems to me to be normative): >>> >>> "For a user-defined conversion operator the result type (i.e., the >>> type to which the operator converts) is part of the mangled name of >>> the function. If the conversion operator is a member template, the >>> result type will appear before the template parameters. There may be >>> forward references in the result type to the template parameters." >>> >> >> -- >> Thanks, >> Pedro Alves >>
[google/gcc-4_9] Backport pending patch to fix demangler crash
Backport pending upstream patch to fix demangler crash.
https://gcc.gnu.org/ml/gcc-patches/2014-05/msg02279.html
This patch is for the google/gcc-4_9 branch.
Google ref: 17891596
-cary
2014-05-27 Pedro Alves
include/
* demangle.h (enum demangle_component_type)
: New value.
2014-05-27 Pedro Alves
libiberty/
* cp-demangle.c (d_demangle_callback, d_make_comp): Handle
DEMANGLE_COMPONENT_CONVERSION.
(is_ctor_dtor_or_conversion): Handle DEMANGLE_COMPONENT_CONVERSION
instead of DEMANGLE_COMPONENT_CAST.
(d_operator_name): Return a DEMANGLE_COMPONENT_CONVERSION
component if handling a conversion.
(d_count_templates_scopes, d_print_comp_inner): Handle
DEMANGLE_COMPONENT_CONVERSION.
(d_print_comp_inner): Handle DEMANGLE_COMPONENT_CONVERSION instead
of DEMANGLE_COMPONENT_CAST.
(d_print_cast): Rename as ...
(d_print_conversion): ... this. Adjust comments.
(d_print_cast): Rewrite - simply print the left subcomponent.
* cp-demint.c (cplus_demangle_fill_component): Handle
DEMANGLE_COMPONENT_CONVERSION.
* testsuite/demangle-expected: Add tests.
Index: include/demangle.h
===
--- include/demangle.h (revision 217320)
+++ include/demangle.h (working copy)
@@ -373,6 +373,10 @@ enum demangle_component_type
/* A typecast, represented as a unary operator. The one subtree is
the type to which the argument should be cast. */
DEMANGLE_COMPONENT_CAST,
+ /* A conversion operator, represented as a unary operator. The one
+ subtree is the type to which the argument should be converted
+ to. */
+ DEMANGLE_COMPONENT_CONVERSION,
/* A nullary expression. The left subtree is the operator. */
DEMANGLE_COMPONENT_NULLARY,
/* A unary expression. The left subtree is the operator, and the
Index: libiberty/cp-demangle.c
===
--- libiberty/cp-demangle.c (revision 217320)
+++ libiberty/cp-demangle.c (working copy)
@@ -526,8 +526,10 @@ d_print_array_type (struct d_print_info
static void
d_print_expr_op (struct d_print_info *, int, const struct demangle_component
*);
-static void
-d_print_cast (struct d_print_info *, int, const struct demangle_component *);
+static void d_print_cast (struct d_print_info *, int,
+ const struct demangle_component *);
+static void d_print_conversion (struct d_print_info *, int,
+ const struct demangle_component *);
static int d_demangle_callback (const char *, int,
demangle_callbackref, void *);
@@ -712,6 +714,9 @@ d_dump (struct demangle_component *dc, i
case DEMANGLE_COMPONENT_CAST:
printf ("cast\n");
break;
+case DEMANGLE_COMPONENT_CONVERSION:
+ printf ("conversion operator\n");
+ break;
case DEMANGLE_COMPONENT_NULLARY:
printf ("nullary operator\n");
break;
@@ -918,6 +923,7 @@ d_make_comp (struct d_info *di, enum dem
case DEMANGLE_COMPONENT_IMAGINARY:
case DEMANGLE_COMPONENT_VENDOR_TYPE:
case DEMANGLE_COMPONENT_CAST:
+case DEMANGLE_COMPONENT_CONVERSION:
case DEMANGLE_COMPONENT_JAVA_RESOURCE:
case DEMANGLE_COMPONENT_DECLTYPE:
case DEMANGLE_COMPONENT_PACK_EXPANSION:
@@ -1209,7 +1215,7 @@ is_ctor_dtor_or_conversion (struct deman
return is_ctor_dtor_or_conversion (d_right (dc));
case DEMANGLE_COMPONENT_CTOR:
case DEMANGLE_COMPONENT_DTOR:
-case DEMANGLE_COMPONENT_CAST:
+case DEMANGLE_COMPONENT_CONVERSION:
return 1;
}
}
@@ -1765,11 +1771,16 @@ d_operator_name (struct d_info *di)
{
struct demangle_component *type;
int was_conversion = di->is_conversion;
+ struct demangle_component *res;
di->is_conversion = ! di->is_expression;
type = cplus_demangle_type (di);
+ if (di->is_conversion)
+ res = d_make_comp (di, DEMANGLE_COMPONENT_CONVERSION, type, NULL);
+ else
+ res = d_make_comp (di, DEMANGLE_COMPONENT_CAST, type, NULL);
di->is_conversion = was_conversion;
- return d_make_comp (di, DEMANGLE_COMPONENT_CAST, type, NULL);
+ return res;
}
else
{
@@ -3863,6 +3874,7 @@ d_count_templates_scopes (int *num_templ
case DEMANGLE_COMPONENT_TEMPLATE_ARGLIST:
case DEMANGLE_COMPONENT_INITIALIZER_LIST:
case DEMANGLE_COMPONENT_CAST:
+case DEMANGLE_COMPONENT_CONVERSION:
case DEMANGLE_COMPONENT_NULLARY:
case DEMANGLE_COMPONENT_UNARY:
case DEMANGLE_COMPONENT_BINARY:
@@ -4962,9 +4974,9 @@ d_print_comp (struct d_print_info *dpi,
d_print_comp (dpi, options, dc->u.s_extended_operator.name);
return;
-case DEMANGLE_COMPONENT_CAST:
+case DEMANGLE_COMPONENT_CONVERSION:
d_append_string (dpi, "operator ");
- d_print_cast (dpi, options, dc);
+ d_print_
Re: RFA/RFC: Add stack recursion limit to libiberty's demangler
> That section is "Writing Robust Programs." Robustness guarantees have > to be different for utilities and servers. A robust server doesn't > crash because of arbitrary user input, but there are servers that > demangle names that are provided by the user. So we need two modes for > the demangler: one that takes anything and sometimes crashes, for > utilities like c++filt, and one that doesn't crash, for servers. And it > seems like that is what Nick is suggesting. In order to handle arbitrary user input without crashing, perhaps the demangler should switch from recursive descent parsing to a state machine, where exhaustion of resources can be handled gracefully. -cary
Re: [1/9][RFC][DWARF] Reserve three DW_OP numbers in vendor extension space
> I'd like to point out that especially the vendor range of DW_OP_* is > extremely scarce resource, we have only a couple of unused values, so taking > 3 out of the remaining unused 12 for a single architecture is IMHO too much. > Can't you use just a single opcode and encode which of the 3 operations it is > in say the low 2 bits of a LEB 128 operand? > We'll likely need to do RSN some multiplexing even for the generic GNU > opcodes if we need just a few further ones (say 0xff as an extension, > followed by uleb128 containing the opcode - 0xff). > In the non-vendor area we still have 54 values left, so there is more space > for future expansion. Most of the Gnu extensions have been adopted into the standard as of DWARF 5: /* GNU extensions. */ DW_OP (DW_OP_GNU_push_tls_address, 0xe0) /* The following is for marking variables that are uninitialized. */ DW_OP (DW_OP_GNU_uninit, 0xf0) DW_OP (DW_OP_GNU_encoded_addr, 0xf1) /* The GNU implicit pointer extension. See http://www.dwarfstd.org/ShowIssue.php?issue=100831.1&type=open . */ DW_OP (DW_OP_GNU_implicit_pointer, 0xf2) /* The GNU entry value extension. See http://www.dwarfstd.org/ShowIssue.php?issue=100909.1&type=open . */ DW_OP (DW_OP_GNU_entry_value, 0xf3) /* The GNU typed stack extension. See http://www.dwarfstd.org/doc/040408.1.html . */ DW_OP (DW_OP_GNU_const_type, 0xf4) DW_OP (DW_OP_GNU_regval_type, 0xf5) DW_OP (DW_OP_GNU_deref_type, 0xf6) DW_OP (DW_OP_GNU_convert, 0xf7) DW_OP (DW_OP_GNU_reinterpret, 0xf9) /* The GNU parameter ref extension. */ DW_OP (DW_OP_GNU_parameter_ref, 0xfa) /* Extensions for Fission. See http://gcc.gnu.org/wiki/DebugFission. */ DW_OP (DW_OP_GNU_addr_index, 0xfb) DW_OP (DW_OP_GNU_const_index, 0xfc) Of these, I think only DW_OP_GNU_uninit and DW_OP_GNU_encoded_addr remain as Gnu extensions. The rest could be deprecated as of DWARF 5, and, if necessary, reused for other purposes in DWARF 6 and later. Depending on how aggressive we want to be with deprecation, we could even declare that they are available for reuse in DWARF 5 and later, as long as the Gnu toolchain uses only the new standard values when generating DWARF 5. That frees up 11 more opcodes. -cary
Re: [1/9][RFC][DWARF] Reserve three DW_OP numbers in vendor extension space
> OK on this proposal and to install this patch to gcc trunk? > > Hi GDB, Binutils maintainer: > > OK on this proposal and install this patch to binutils-gdb master? > > include/ > 2016-11-29 Richard Earnshaw > Jiong Wang > > * dwarf2.def (DW_OP_AARCH64_operation): Reserve the number 0xea. This is OK, but: +/* AARCH64 extensions. + DW_OP_AARCH64_operation takes one mandatory unsigned LEB128 operand. + Bits[6:0] of this operand is the action code, all others bits are initialized + to 0 except explicitly documented for one action. Please refer AArch64 DWARF + ABI documentation for details. */ Is it possible to include a stable URL that points to the ABI document? -cary
Re: [PATCH] Add linker plugin API for processing plugin-added input files
> 2017-09-21 Stephen Crane > > * plugin-api.h: Add new hook to the plugin transfer vector to > support assigning plugin-generated sections to unique output > segments. > (ld_plugin_register_new_input): New hook. > (ld_plugin_tag): Add LDPT_REGISTER_NEW_INPUT_HOOK. > (ld_plugin_tv): Add tv_register_new_input. This addition to the plugin API makes sense to me, but I'd appreciate Sri's input. -cary
Re: [PR63238] output alignment debug information
> for gcc/ChangeLog > > PR debug/63238 > * dwarf2out.c (clone_as_declaration): Drop DW_AT_alignment. > (add_alignment_attribute): New. > (base_type_die): Add alignment attribute. > (subrange_type_die): Likewise. > (modified_type_die): Likewise. > (gen_array_type_die): Likewise. > (gen_descr_array_type_die: Likewise. > (gen_enumeration_type_die): Likewise. > (gen_subprogram_die): Likewise. > (gen_variable_die): Likewise. > (gen_field_die): Likewise. > (gen_ptr_to_mbr_type_die): Likewise. > (gen_struct_or_union_type_die): Likewise. > (gen_subroutine_type_die): Likewise. > (gen_typedef_die): Likewise. > (base_type_cmp): Compare alignment attribute. This is OK so far, but the DW_AT_alignment attribute also needs to be added to the checksum computation in die_checksum and die_checksum_ordered. -cary
Re: [PR63238] output alignment debug information
>> This is OK so far, but the DW_AT_alignment attribute also needs to be >> added to the checksum computation in die_checksum and >> die_checksum_ordered. > > Thanks. I see what to do in die_checksum_ordered, but die_checksum? It > seems to handle attributes by value class, and AFAICT the classes that > DW_AT_alignment could use are already covered. What am I missing? > > Here's a patch I'm about to start testing. Does it look ok? Sorry, I read this while I wasn't in a position to reply, then totally forgot about it. Yes, you're right about die_checksum, sorry. And, for the record, it looks OK. -cary
[committed patch] Sync top-level configure.ac with binutils-gdb
I'm committing this patch to sync the top-level configure with binutils-gdb.
-cary
2016-03-17 Cary Coutant
* configure.ac: Add mips and s390 to the gold target check.
* configure: Regenerate.
Index: configure
===
--- configure (revision 234307)
+++ configure (working copy)
@@ -2973,7 +2973,7 @@ case "${ENABLE_GOLD}" in
# Check for target supported by gold.
case "${target}" in
i?86-*-* | x86_64-*-* | sparc*-*-* | powerpc*-*-* | arm*-*-* \
-| aarch64*-*-* | tilegx*-*-*)
+| aarch64*-*-* | tilegx*-*-* | mips*-*-* | s390*-*-*)
configdirs="$configdirs gold"
if test x${ENABLE_GOLD} = xdefault; then
default_ld=gold
Index: configure.ac
===
--- configure.ac(revision 234307)
+++ configure.ac(working copy)
@@ -351,7 +351,7 @@ case "${ENABLE_GOLD}" in
# Check for target supported by gold.
case "${target}" in
i?86-*-* | x86_64-*-* | sparc*-*-* | powerpc*-*-* | arm*-*-* \
-| aarch64*-*-* | tilegx*-*-*)
+| aarch64*-*-* | tilegx*-*-* | mips*-*-* | s390*-*-*)
configdirs="$configdirs gold"
if test x${ENABLE_GOLD} = xdefault; then
default_ld=gold
Re: [1/9][RFC][DWARF] Reserve three DW_OP numbers in vendor extension space
How about if instead of special DW_OP codes, you instead define a new virtual register that contains the mangled return address? If the rule for that virtual register is anything other than DW_CFA_undefined, you'd expect to find the mangled return address using that rule; otherwise, you would use the rule for LR instead and expect an unmangled return address. The earlier example would become (picking an arbitrary value of 120 for the new virtual register number): .cfi_startproc 0x0 paciasp (this instruction sign return address register LR/X30) .cfi_val 120, DW_OP_reg30 0x4 stp x29, x30, [sp, -32]! .cfi_offset 120, -16 .cfi_offset 29, -32 .cfi_def_cfa_offset 32 0x8 add x29, sp, 0 Just a suggestion... -cary On Wed, Nov 16, 2016 at 6:02 AM, Jakub Jelinek wrote: > On Wed, Nov 16, 2016 at 02:54:56PM +0100, Mark Wielaard wrote: >> On Wed, 2016-11-16 at 10:00 +, Jiong Wang wrote: >> > The two operations DW_OP_AARCH64_paciasp and DW_OP_AARCH64_paciasp_deref >> > were >> > designed as shortcut operations when LR is signed with A key and using >> > function's CFA as salt. This is the default behaviour of return address >> > signing so is expected to be used for most of the time. >> > DW_OP_AARCH64_pauth >> > is designed as a generic operation that allow describing pointer signing on >> > any value using any salt and key in case we can't use the shortcut >> > operations >> > we can use this. >> >> I admit to not fully understand the salting/keying involved. But given >> that the DW_OP space is really tiny, so we would like to not eat up too >> many of them for new opcodes. And given that introducing any new DW_OPs >> using for CFI unwinding will break any unwinder anyway causing us to >> update them all for this new feature. Have you thought about using a new >> CIE augmentation string character for describing that the return >> address/link register used by a function/frame is salted/keyed? >> >> This seems a good description of CIE records and augmentation >> characters: http://www.airs.com/blog/archives/460 >> >> It obviously also involves updating all unwinders to understand the new >> augmentation character (and possible arguments). But it might be more >> generic and saves us from using up too many DW_OPs. > > From what I understood, the return address is not always scrambled, so > it doesn't apply to the whole function, just to most of it (except for > an insn in the prologue and some in the epilogue). So I think one op is > needed. But can't it be just a toggable flag whether the return address > is scrambled + some arguments to it? > Thus DW_OP_AARCH64_scramble .uleb128 0 would mean that the default > way of scrambling starts here (if not already active) or any kind of > scrambling ends here (if already active), and > DW_OP_AARCH64_scramble .uleb128 non-zero would be whatever encoding you need > to represent details of the less common variants with details what to do. > Then you'd just hook through some MD_* macro in the unwinder the > descrambling operation if the scrambling is active at the insns you unwind > on. > > Jakub
Re: [PATCH] Add extensions to dwarf2.def
> I'm currently working on migrating debugging information for Ada from GNAT > encodings to standard DWARF. At the moment, I have worked on two topics that > I believe are not (completely) supported in standard DWARF: > > - fixed point types with arbitrary scale factors; > - scalar types with biased representations. > > My goal is to submit an issue on dwarfstd.org in an attempt to introduce > these extensions to the next DWARF standard. Before that, though, I would > like to make sure that these extensions actually fit the need by having them > supported both in GCC and GDB. > > The two attached patches make these extensions "public" so that no other > vendor-specific tags/attributes conflict with them in the future. I cannot > submit the patches that actually use these right now because I need first to > port them from the 4.9 branch onto mainline (I hope I will be able to do > this on early July). > > May I commit them? > > I also attached two documents that describe how to use these extensions. I > guess this should go to the wiki just like for DW_AT_GNAT_descriptive_type > (https://gcc.gnu.org/wiki/DW_AT_GNAT_descriptive_type). I will do this if > the patches are integrated. > > Thank you in advance! > > include/ > * dwarf2.def (DW_TAG_GNU_rational_constant): New tag. > (DW_AT_GNU_numerator, DW_AT_GNU_denominator): New attributes. I don't think you really need a new TAG here -- DW_TAG_constant could just as easily take DW_AT_GNU_numerator and DW_AT_GNU_denominator as an alternative to DW_AT_const_value. I'm not really sure why DW_AT_small was defined to refer to a DW_TAG_constant DIE rather than just providing the constant as the attribute value. It would seem more efficient, space-wise, to have a DW_AT_scale attribute that would provide a multiplicative scale factor, and an optional DW_AT_scale_divisor to provide the denominator if necessary. Another, perhaps far-fetched, alternative would be to introduce a new form that would represent a rational constant as two unsigned LEB128 values, and allow that form for DW_AT_const_value and/or for DW_AT_small. For now, I'd suggest going with your proposal, except use the existing DW_TAG_constant instead of a new TAG. (I.e., just add the two new DW_AT_numerator and DW_AT_denominator attributes.) > include/ > * dwarf2.def (DW_AT_GNU_bias): New attribute. This is OK. Looks like a good idea to me. -cary
Re: [RFC] COMDAT Safe Module Level Multi versioning
Based on Richard's suggestion, I have a patch to localize comdat functions which seems like a very effective solution to this problem. The text size increase is limited to the extra comdat copies generated for the specialized modules (modules with unsafe options) which is usually only a few. Since -fweak does something similar for functions, I have called the new option -fweak-comdat-functions. This does not apply to virtual comdat functions as their addresses can always be leaked via the vtable. Using this flag with virtual functions generates a warning. +fweak-comdat-functions +C++ Var(flag_weak_comdat_functions) Init(1) +Specific to comdat functions(-fno-weak-comdat-functions : Localize Comdat Functions). +With -fno-weak-comdat-functions, virtual comdat functions are still linked as +weak functions. With -fno-weak-comdat-functions, the address of the comdat +functions that are localized will be unique and this can cause unintended +behavior when addresses of comdat functions are used. Is one of those "With -fno-weak-comdat-functions" supposed to be "With -fweak-comdat-functions"? This description doesn't really say what the flag (without the "no") does, and doesn't explain what "localize" means. +@item -fno-weak-comdat-functions +@opindex fno-weak-comdat-functions +Do not use weak symbol support for comdat non-virtual functions, even if it +is provided by the linker. By default, G++ uses weak symbols if they are +available. This option is useful when comdat functions generated in certain +compilation units need to be kept local to the respective units and not exposed +globally. This does not apply to virtual comdat functions as their pointers +may be taken via virtual tables. This can cause unintended behavior if +the addresses of comdat functions are used. It's not really the "weak" that is causing the problem -- it's the "comdat". What the option really is doing is making the functions static rather than comdat. (It's all gated under flag_weak because weak symbols are the fall-back to link-once and comdat symbols.) I'd suggest phrasing this more in terms of static vs. comdat. This looks like the right way to go, though. -cary
Re: [RFC] COMDAT Safe Module Level Multi versioning
> +@item -fno-weak-comdat-functions > +@opindex fno-weak-comdat-functions > +Do not use weak symbol support for comdat non-virtual functions, even if it > +is provided by the linker. By default, G++ uses weak symbols if they are > +available. This option is useful when comdat functions generated in certain > +compilation units need to be kept local to the respective units and not > exposed > +globally. This does not apply to virtual comdat functions as their pointers > +may be taken via virtual tables. This can cause unintended behavior if > +the addresses of comdat functions are used. > > It's not really the "weak" that is causing the problem -- it's the > "comdat". What the option really is doing is making the functions > static rather than comdat. (It's all gated under flag_weak because > weak symbols are the fall-back to link-once and comdat symbols.) I'd > suggest phrasing this more in terms of static vs. comdat. Oh, also, I'd suggest clarifying what you mean by "if the addresses of comdat functions are used". I think what you really mean here is "if pointers to the [now non-comdat] functions are compared for equality." -cary
Re: [RFC] COMDAT Safe Module Level Multi versioning
> Thanks, will make those changes. Do you recommend a different name > for this flag like -fmake-comdat-functions-static? Well, the C++ ABI refers to this as "vague linkage." It may be a bit too long or too ABI-specific, but maybe something like -f[no-]use-vague-linkage-for-functions or -f[no-]functions-vague-linkage? And perhaps note in the doc that using this option may technically break the C++ ODR, so it should be used only when you know what you're doing. -cary
Re: [Dwarf Fission] Implement Fission Proposal (issue6305113)
> +/* %:replace-extension spec function. Replaces the extension of the
> + first argument with the second argument. */
> +
> +const char *
> +replace_extension_spec_func (int argc, const char **argv)
> +{
> + char *name;
> + char *p;
> + char *result;
> +
> + if (argc != 2)
> +fatal_error ("too few arguments to %%:replace-extension");
> +
> + name = xstrdup (argv[0]);
> + p = strrchr (name, '.');
> + if (p != NULL)
> + *p = '\0';
> +
> + result = concat (name, argv[1], NULL);
> +
> + free (name);
> + return result;
> +}
This doesn't do the right thing when there is no '.' in the last
component of the path. It should look for the last DIR_SEPARATOR,
then search for the last '.' after that.
> +/* Describe an entry into the .debug_addr section. */
> +
> +enum ate_kind {
> + ate_kind_rtx,
> + ate_kind_rtx_dtprel,
> + ate_kind_label
> +};
> +
> +typedef struct GTY(()) addr_table_entry_struct {
> + enum ate_kind kind;
> + unsigned int refcount;
> + unsigned int index;
> + union addr_table_entry_struct_union
> +{
> + rtx GTY ((tag ("ate_kind_rtx"))) rtl;
> + char * GTY ((tag ("ate_kind_label"))) label;
> +}
> + GTY ((desc ("%1.kind"))) addr;
When kind == ate_kind_rtx_dtprel, we use the rtl field. I think this needs
to be covered for GC to work. As far as I know, gengtype doesn't support
multiple tags for one union member, so I think it needs to be something
like this:
union addr_table_entry_struct_union
{
rtx GTY ((tag ("0"))) rtl;
char * GTY ((tag ("1"))) label;
}
GTY ((desc ("(%1.kind == ate_kind_label)"))) addr;
> +static void add_AT_lbl_id (dw_die_ref, enum dwarf_attribute, const char *,
> + bool);
It turns out we never call add_AT_lbl_id with force_direct == true.
I don't think it's necessary to add this parameter here.
> +/* enum for tracking thread-local variables whose address is really an offset
> + relative to the TLS pointer, which will need link-time relocation, but
> will
> + not need relocation by the DWARF consumer. */
> +
> +enum dtprel_bool
> + {
> +dtprel_false = 0,
> +dtprel_true = 1
> + };
Extra indentation here.
> +static inline enum dwarf_location_atom
> +dw_addr_op (enum dtprel_bool dtprel)
> +{
> + if (dtprel == dtprel_true)
> +return (dwarf_split_debug_info ? DW_OP_GNU_const_index
> +: (DWARF2_ADDR_SIZE == 4 ? DW_OP_const4u : DW_OP_const8u));
> + else
> +return (dwarf_split_debug_info ? DW_OP_GNU_addr_index : DW_OP_addr);
Unnecessary parentheses here.
> +/* Return the index for any attribute that will be referenced with a
> + DW_FORM_GNU_addr_index. Strings have their indices handled differently to
> + account for reference counting pruning. */
> +
> +static inline unsigned int
> +AT_index (dw_attr_ref a)
> +{
> + if (AT_class (a) == dw_val_class_str)
> +return a->dw_attr_val.v.val_str->index;
> + else if (a->dw_attr_val.val_entry != NULL)
> +return a->dw_attr_val.val_entry->index;
> + return NOT_INDEXED;
> +}
The comment seems out of date. DW_FORM_GNU_str_index should also be
mentioned, and it doesn't look like strings have their indices handled
differently (at least not here).
> +static void
> +remove_addr_table_entry (addr_table_entry *entry)
> +{
> + addr_table_entry *node;
> +
> + gcc_assert (dwarf_split_debug_info && addr_index_table);
> + node = (addr_table_entry *) htab_find (addr_index_table, entry);
> + node->refcount--;
> + /* After an index is assigned, the table is frozen. */
> + gcc_assert (node->refcount > 0 || node->index == NO_INDEX_ASSIGNED);
This shouldn't ever be called after we've assigned any indexes at all,
so I think it's always safe to asser that node->index == NO_INDEX_ASSIGNED.
We can also assert that the ref count should never go negative, so I think
you can rewrite this assert as:
gcc_assert (node->refcount >= 0 && node->index == NO_INDEX_ASSIGNED);
> @@ -21215,7 +22086,7 @@ prune_unused_types_update_strings (dw_die_ref die)
> *slot = s;
> }
>}
> -}
> + }
Accidental extra space?
> +static void
> +index_location_lists (dw_die_ref die)
> +{
> + dw_die_ref c;
> + dw_attr_ref a;
> + unsigned ix;
> +
> + FOR_EACH_VEC_ELT (dw_attr_node, die->die_attr, ix, a)
> +if (AT_class (a) == dw_val_class_loc_list)
> + {
> +dw_loc_list_ref list = AT_loc_list (a);
> +dw_loc_list_ref curr;
> +for (curr = list; curr != NULL; curr = curr->dw_loc_next)
> + {
> +/* Don't index an entry that has already been indexed
> + or won't be output. */
> +if (curr->begin_entry != NULL
> + || (strcmp (curr->begin, curr->end) == 0 && !curr->force))
> + continue;
Check the indentation here -- looks like these last two lines are missing
a space.
OK for trunk with these changes. Thanks!
-cary
Re: [PATCH] Fix up assembly thunks with -gstabs+ (PR debug/54499)
jakub> 2012-11-08 Jakub Jelinek
jakub>
jakub> PR debug/54499
jakub> * cgraphunit.c (assemble_thunk): Don't call source_line debug hook
jakub> here, instead call insn_locations_{init,finalize} and initialize
jakub> prologue_location.
jakub>
jakub> * g++.dg/debug/pr54499.C: New test.
Thanks, Jakub, this does look like a better fix. I had tried something
like this, but it didn't work -- I was missing the _init and _finalize
calls, and I didn't set prologue_location either.
rth> I recall mentioning at the time that I thought Cary's approach
was error-prone.
Well, yes, you did, but then you backed off after I tested an ARM compiler:
On Mon, Aug 6, 2012 at 2:53 PM, Richard Henderson wrote:
> On 08/06/2012 02:45 PM, Cary Coutant wrote:
>> Do you still have concerns about the patch?
>
> Nope. I'd mis-remembered from whence things get finalized.
>
> Revised patch is ok.
I do agree with you, though, that it would still be better to treat
thunks as first-class objects.
-cary
Re: [Bug debug/55328] ICE: in output_addr_table_entry, at dwarf2out.c:21780 with -gsplit-dwarf
> Enclosed is a simple fix for this ICE--address_table_entries that have > zero refcounts shouldn't be assigned an index. > > OK for trunk? > > Sterling > > 2012-11-14 Sterling Augustine > > * dwarf2out.c (index_address_table_entry): Check a node's refcount. Add PR debug/55328 to the ChangeLog entry. OK for trunk with that change. -cary
Re: [Google 4.7] Backport upstream fission changes (issue6852101)
> 2012-11-27 Sterling Augustine > > Backport changes to fission implementation required by > trunk. See > http://gcc.gnu.org/ml/gcc-patches/2012-10/msg02684.html and > susbsequent messages for a full description of what needed to > change for it to be accepted into trunk. Most of the original > changes were already in google-4_7, so this is just the diff > between the two implementations. > > These changes to make the resulting debug info smaller, so are useful > to have on this branch. > > * dwarf2out.h (addr_table_entry_struct): Forward declare. > (dw_val_struct): Change name and associated type of field val_index > to val_entry. > * dwarf2out.c (NOT_INDEXED, NO_INDEX_ASSIGNED, UNRELOCATED_OFFSET, > RELOCATED_OFFSET): New defines. > (ate_kind): New enum with enumerators ate_kind_rtx, > ate_kind_rtx_dtprel, ate_kind_label. > (addr_table_entry_struct, addr_table_entry): New structure and type. > (dw_loc_list_struct): Change field name from begin_index to > begin_entry, likewise with the type. > (new_loc_descr, build_cfa_loc, new_addr_loc_descr, add_AT_flag, > add_AT_int, add_AT_unsigned, add_AT_double, add_AT_vec, > add_AT_string, add_AT_die_ref, add_AT_fde_ref, > add_AT_loc, add_AT_loc_list, add_AT_addr, add_AT_file, > add_AT_vms_delta, > add_AT_lbl_id, add_AT_lineptr, add_AT_macptr, add_AT_offset): > Change field name from val_index to val_entry, and intialize properly. > (size_of_loc_descr, output_loc_operands): Add assertion and update > call. > (set_AT_index): Remove obsolete function. > (remove_addr_table_entry, add_AT_lbl_id, > add_AT_range_list, add_ranges_by_labels): Update prototypes, adjust > all calls in file. > (dtprel_bool): New enum with dtprel_false and dtprel_true. > (dw_addr_op, new_addr_loc_descr): Update functions to use dtprel_bool, > plus additional logic to handle val_entries. Use RELOCATED_OFFSET > and UNRELOCATED_OFFSET. > (AT_index, output_loc_list): Update comment. Update logic to handle > val_entry. > (add_AT_low_high_pc): Update comment. Change field name from > val_index to val_entry, and call add_addr_table entry. > (indirect_string_node, index_string_table): Delete obsolete types, > variables and tables. > (AT_string_form): Move most logic to ... > (find_string_form): ... here and ... > (set_indirect_string): ... here. New function. > (addr_index_table): Change type from vector to hash table. > (addr_table_entry_do_hash, addr_table_entry_eq, init_addr_table_entry, > index_addr_table_entry): New functions. > (add_addr_table_entry): Update prototype. Convert logic to > hash tables. > (remove_loc_list_addr_table_entries): Convert logic to hash tables. > (size_of_die, value_format, output_attr_index_or_value): Use > NOT_INDEXED and NO_INDEX_ASSIGNED. > (output_range_list_offset): Use RELOCATED_OFFSET. Update comment. > (output_comdat_type_unit): Check OBJECT_FORMAT_ELF. > (add_ranges_by_labels, loc_descriptor, mem_loc_descriptor, > loc_list_from_tree, add_const_value_attribute): Update comment. Use > dtprel_bool enumerators. > (output_macinfo_op): Fix FIXME by handling DW_FORM_strp's and > DW_FORM_GNU_str_index. > (save_macinfo_strings, index_string, output_index_string_offset, > output_index_string, output_indirect_strings, > output_addr_table_entry): > New functions. > (output_indirect_string): Check refcount. > (output_addr_table): Update logic for hash tables. > (resolve_addr_in_expr, resolve_addr, hash_loc_operands, > compare_loc_operands, index_location_lists): Update logic for > val_entry. > (dwarf2out_finish): Update logic for val_entry. Call > htab_traverse_noresize multiple times, save macinfo_strings, > index_string output_addr_table, output_indirect_strings. Add comments. > Remove calls to output_addr_table, output_index_strings, > and index_location_lists. This is OK for the google/gcc-4_7 branch. Thanks! -cary
Re: [PATCH] Improve debug info for various cases where we drop location info on the floor (PR debug/55608)
> This patch adds DW_AT_const_value or DW_AT_location for unused static vars > (thus, not really modified and their DECL_INITIAL can be used for > location/constant value info), and optimizes various cases using > DW_OP_GNU_implicit_pointer (e.g. DW_OP_addr DW_OP_stack_value > where symbol is either an optimized away static var (but with DW_AT_location > or DW_AT_const_value on it's DIE), which would be otherwise dropped > to avoid referencing non-existent symbol, can be replaced with > DW_OP_GNU_implicit_pointer to that DIE. Or, if symbol above is a constant > string > literal, we can create DW_TAG_dwarf_procedure with DW_AT_location > DW_OP_implicit_value (and, surprisingly, GDB handles that out of the box). > > In cc1plus .debug_loc grew with this by about 3% and .debug_info grew by > 0.25%, in libstdc++ the changes were in the noise. Based on my reading of PR 55608, this isn't really a regression, and it's definitely not a regression from 4.7 to 4.8. While the patch looks OK to me, the size increase in .debug_loc and the scope of the change suggest that this would be more appropriate for when Stage 1 reopens. -cary
Re: [patch, mips, debug] Fix PR 54061, mips compiler aborts in testsuite
> 2012-12-07 Steve Ellcey > > PR target/54061 > * rtl.h (IGNORED_DWARF_REGNUM): New. > * dwarfwout.c (reg_loc_descriptor): Check for IGNORED_DWARF_REGNUM. Typo: s/dwarfwout/dwarf2out/ > (mem_loc_descriptor): Ditto. > * config/mips/mips.c (mips_option_override): Set mips_dbx_regno > coprocessor entries to IGNORED_DWARF_REGNUM. -cary
Re: [patch, mips, debug] Fix PR 54061, mips compiler aborts in testsuite
> 2012-12-07 Steve Ellcey > > PR target/54061 > rtl.h (IGNORED_DWARF_REGNUM): New. > * dwarf2out.c (reg_loc_descriptor): Check for IGNORED_DWARF_REGNUM. > (mem_loc_descriptor): Ditto. > * config/mips/mips.h (ALL_COP_REG_FIRST): New. > (ALL_COP_REG_LAST): New. > (ALL_COP_REG_NUM): Redefine using above macros. > * config/mips/mips.c (mips_option_override): Set mips_dbx_regno > coprocessor entries to IGNORED_DWARF_REGNUM. The dwarf2out.c parts are OK. -cary
Re: [DWARF] Fix multiple register spanning location.
2013-04-26 Christian Bruel
* dwarf2out.c (multiple_reg_loc_descriptor): Use DBX_REGISTER_NUMBER
for spaning registers.
s/spaning/spanning/
Index: dwarf2out.c
===
--- dwarf2out.c (revision 198287)
+++ dwarf2out.c (working copy)
@@ -10656,7 +10656,8 @@ multiple_reg_loc_descriptor (rtx rtl, rtx regs,
{
dw_loc_descr_ref t;
- t = one_reg_loc_descriptor (REGNO (XVECEXP (regs, 0, i)),
+ reg = REGNO (XVECEXP (regs, 0, i));
+ t = one_reg_loc_descriptor (DBX_REGISTER_NUMBER (reg),
VAR_INIT_STATUS_INITIALIZED);
add_loc_descr (&loc_result, t);
size = GET_MODE_SIZE (GET_MODE (XVECEXP (regs, 0, 0)));
How about using dbx_reg_number (XVECEXP (regs, 0, i)) instead? The
bare use of DBX_REGISTER_NUMBER earlier in that function is protected
by a gcc_assert, but this one isn't.
-cary
Re: [Google 4.8 Dwarf] Backport .debug_str in .o files with -gsplit-dwarf from trunk (issue9052046)
> The enclosed patch puts strings in the .debug_str section in the .o > file with -gsplit-dwarf. It is backported from from trunk (which, in turn, > was ported from google_4.7). > > http://gcc.gnu.org/ml/gcc-patches/2013-04/msg01355.html > > The patch itself is no different from the one finally applied to trunk, > except several hunks have moved a few lines. > > OK for gcc/google/gcc-4_8? OK. -cary
Re: [PATCH] Fix incorrect discriminator assignment.
> gcc/ChangeLog: > > * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno. > (locus_descrim_hasher::equal): Likewise. > (next_discriminator_for_locus): Likewise. > (assign_discriminator): Add return value. > (make_edges): Assign more discriminator if necessary. > (make_cond_expr_edges): Likewise. > (make_goto_expr_edges): Likewise. > > gcc/testsuite/ChangeLog: > > * gcc.dg/debug/dwarf2/discriminator.c: New Test. Looks OK to me, but I can't approve patches for tree-cfg.c. Two comments: (1) In the C++ conversion, it looks like someone misspelled "discrim" in "locus_descrim_hasher". (2) Is this worth fixing in trunk when we'll eventually switch to a per-instruction discriminator? -cary
