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: [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: 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: [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
[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
>> 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
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] 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] 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
>> 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
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: [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: [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
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
[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
[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: [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: [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
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: [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
[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: [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
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: [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}
> 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: 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: [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
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] 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: [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: [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
>> > 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
>> +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: 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: [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: [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] 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
[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: [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 >>
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
[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
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
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
> 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 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: [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: [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: [PATCH 2/2] PR debug/63240 Add DWARF representation for C++11 defaulted member function.
> O. Then I was indeed wrong and defaulted does not impact ABI at all. > At least that is one worry less for the abi checkers :) As Siva mentioned, it does in fact impact the ABI. A class with a non-trivial destructor is not a POD, and affects the calling convention, so the debugger needs to know the difference. C++ ABI reference here: http://mentorembedded.github.io/cxx-abi/abi.html#return-value I've submitted a DWARF proposal to document the use of the DW_AT_artificial attribute on a default copy constructor or destructor. -cary
Re: Avoid privatization of TLS variables
> Thank you! Now when I have your attention, perhaps we could discuss the > original > motivation of the change that exposed this bug. > I was building libreoffice with profile feedback and I run into a message > > cannot load any more object with static TLS > > that took me a while to track as I did not see where static TLS is comming > out. > Ian pointed out to me that static variables with TLS storage also consume > static TLS even if they are in dynamic model. This is why I disabled > localization. Is there better way to handle this? As I understand it, if you compile with -fpic, you shouldn't see any static TLS. The compiler should only use the Local Exec model for static/hidden variables in non-PIC compiles. > Note that the variable __gcov_indirect_call_counters_ltopriv was added to > work around > https://sourceware.org/bugzilla/show_bug.cgi?id=14342 > that seems to be fixed. I would like to drop the hack (that will also make > profiling to work with current golds again), but I think I would like to > document > when the bug went away, becuase it is only bit over a year now. > > Any idea when it was fixed? I think it was fixed with this patch: https://sourceware.org/ml/binutils/2013-06/msg00139.html I guess I should close that bug. -cary
Re: Avoid privatization of TLS variables
The plugin API doesn't have a way to mark a symbol as TLS, but it doesn't really matter since the linker simply overrides the placeholder from the claimed file with the symbol provided in the replacement. Unfortunately, I excluded common symbols from this logic in gold, so the symbol isn't getting marked as TLS COMMON even when we see the real ELF file. I just need to remind myself why the exclusion is there and figure out how to fix it. I think all the information we need is there, so no changes to the plugin API should be necessary. -cary On Thu, Sep 25, 2014 at 9:29 AM, Jan Hubicka wrote: >> On Thu, Sep 25, 2014 at 8:37 AM, H.J. Lu wrote: >> > On Thu, Sep 25, 2014 at 8:24 AM, Ian Lance Taylor wrote: >> >> On Wed, Sep 24, 2014 at 6:58 PM, Jan Hubicka wrote: >> >>> >> >>>b: 00 00 >> >>> 9: R_X86_64_TPOFF32 >> >>> __gcov_indirect_call_counters_ltopriv >> >> >> >> Look at the .o file where __gcov_indirect_call_counters_ltopriv is >> >> defined. That .o file must have the symbol marked as STT_TLS and it >> >> must be defined in a section with the SHF_TLS flag. If that is not >> >> true, then that is your problem. >> > >> > SHF_TLS isn't required. >> > >> > 16: 0008 8 TLS GLOBAL HIDDEN COM >> > __gcov_indirect_call_counters_ltopriv >> > 17: 0008 8 TLS GLOBAL HIDDEN COM >> > __gcov_indirect_call_callee_ltopriv >> > >> > are also sufficient. >> >> I can create a .o file with a hidden common symbol, but I can't >> recreate the problem. When I try, gold creates a TLS section and TLS >> segment itself. >> >> How exactly is gold being invoked? > > It seems to happen with LTO compilation only, just build mainline tree > and try the original testcase. > > Honza >> >> Ian
[google/4_9] Add a new flag bit to .debug_gnu_pubnames entries
This patch is for the google/gcc-4_9 branch only. Use one of the reserved bits in the flag byte to tell gold which symbols in the pubnames table can be entered just once in the .gdb_index (rather than listing all CUs in which a given symbol appears). We set this bit for class/struct types and namespaces only. -cary 2014-09-11 Cary Coutant gcc/ * dwarf2out.c (output_pubname): Use a reserved bit in the flags byte to tell gold it's OK to keep just one CU in the CU list for this index entry. Index: dwarf2out.c === --- dwarf2out.c (revision 215195) +++ dwarf2out.c (working copy) @@ -9181,6 +9181,16 @@ add_pubtype (tree decl, dw_die_ref die) } } +/* We use one of the reserved bits in the flags byte to tell the linker + which index entries do not need to list all CUs that contain the symbol. */ + +#define GDB_INDEX_RESERVED_SINGLE_CU 8 +#define GDB_INDEX_RESERVED_SET_VALUE(cu_index, value) \ + do { \ +(cu_index) |= (((value) & GDB_INDEX_RESERVED_MASK) \ + << GDB_INDEX_RESERVED_SHIFT); \ + } while (0) + /* Output a single entry in the pubnames table. */ static void @@ -9227,6 +9237,9 @@ output_pubname (dw_offset die_offset, pu GDB_INDEX_SYMBOL_STATIC_SET_VALUE(flags, is_static); break; case DW_TAG_namespace: + GDB_INDEX_SYMBOL_KIND_SET_VALUE(flags, GDB_INDEX_SYMBOL_KIND_TYPE); + GDB_INDEX_RESERVED_SET_VALUE(flags, GDB_INDEX_RESERVED_SINGLE_CU); + break; case DW_TAG_imported_declaration: GDB_INDEX_SYMBOL_KIND_SET_VALUE(flags, GDB_INDEX_SYMBOL_KIND_TYPE); break; @@ -9236,6 +9249,7 @@ output_pubname (dw_offset die_offset, pu case DW_TAG_union_type: case DW_TAG_enumeration_type: GDB_INDEX_SYMBOL_KIND_SET_VALUE(flags, GDB_INDEX_SYMBOL_KIND_TYPE); + GDB_INDEX_RESERVED_SET_VALUE(flags, GDB_INDEX_RESERVED_SINGLE_CU); if (!is_cxx () && !is_java ()) GDB_INDEX_SYMBOL_STATIC_SET_VALUE(flags, 1); break;
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: [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
[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.
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); }
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
Re: [PATCH 1/6] RTL & dwarf2out changes
>> In the case of loc_checksum(), we're tied to MD5 by the DWARF >> standard. Otherwise, we could just rewrite it to use inchash >> throughout. > > I'm not sure I understand the motivation. If gcc hashes in > gcc specific stuff (and this hash, even before my changes is) > then the output can never be re-created by anything but gcc. > > If the standard just wants a well hashed number at the end > then any good hash should do. It's complicated. The DWARF standard specifies how the signature for a type unit (-fdebug-types-section) is computed, using MD5. There are occasional location expressions in a type unit, but the only ones we should see when computing a type signature are special-cased, and we should never actually get to where hash_loc_operands() is called. If one does slip through, it's not fatal -- we'll just generate a type signature that doesn't conform to the standard, and we may miss an opportunity for link-time type de-duplication. For both -feliminate-dwarf2-dups and -gsplit-dwarf, though, we also compute DIE signatures using the same code, and in these cases, we may see location expressions that need hash_loc_operands(). These signatures are not specified by the DWARF standard, so it's reasonable (IMO) to reuse the existing hashing routine in that case. These signatures are used for de-duplication, for fast lookup, and for disambiguation where two CUs have the same DW_AT_name, so the loss of information is not critical. > I haven't checked it for this case, but if the hashing shows > up in profiles it may be worth using a faster but non secure > hash. > > Anyways I can drop the comment if you don't agree with it. Thanks, please do. It does make sense, even if there's a theoretically better way to do it. -cary
Re: [PATCH 1/6] RTL & dwarf2out changes
> + /* ??? MD5 of another hash doesn't make a lot of sense... */ > + hash = hstate.end(); >CHECKSUM (hash); [citation needed] I don't see why you think that. Maybe it'd be nicer if we could use hash_loc_operands() to feed its input directly into the MD5 checksum, but I think in this case it's perfectly fine to use the hash instead, in order to avoid reimplementing a rather substantial function that already exists. Maybe we could make hash_loc_operands() a template that can be used as part of either inchash or MD5? In the case of loc_checksum(), we're tied to MD5 by the DWARF standard. Otherwise, we could just rewrite it to use inchash throughout. -cary
Re: [PATCH 2/3] PR other/61321 - demangler crash on casts in template parameters
> 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. 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." -cary
[patch] [4.9] Backport fix for ICEs with -gsplit-dwarf
I've backported this patch from trunk at r212211. Committed to gcc-4_9 at r212434. https://gcc.gnu.org/ml/gcc-patches/2014-07/msg00076.html -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.
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 ()); >
[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: [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
[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: [PATCH 2/3] PR other/61321 - demangler crash on casts in template parameters
> Fix this by adding a new DEMANGLE_COMPONENT_CONVERSION component type, > which does what DEMANGLE_COMPONENT_CAST does today, and making > DEMANGLE_COMPONENT_CAST just simply print its component subtree. > > I think we could instead reuse DEMANGLE_COMPONENT_CAST and in > d_print_comp_inner still do: > > @@ -5001,9 +5013,9 @@ d_print_comp_inner (struct d_print_info *dpi, int > options, > d_print_comp (dpi, options, dc->u.s_extended_operator.name); > return; > > case DEMANGLE_COMPONENT_CAST: >d_append_string (dpi, "operator "); > - d_print_cast (dpi, options, dc); > + d_print_conversion (dpi, options, dc); >return; > > leaving the unary cast case below calling d_print_cast, but seems to > me that spliting the component types makes it easier to reason about > the code. I agree. > libiberty/ > 2014-05-27 Pedro Alves > > PR other/61321 > PR other/61233 > * demangle.h (enum demangle_component_type) > : New value. > * 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. Looks good to me. Thanks! Ian, does this look good to you? -cary
Re: [patch] PR debug/61013: Change -g so that it will override -g1 but not -g3
>> PR debug/61013 >> * opts.c (common_handle_option): Don't special-case "-g". >> (set_debug_level): Default to at least level 2 with "-g". > > Ok. Thanks, Forgot to ask -- OK to backport to the 4.9 branch? -cary
[patch] PR debug/61013: Change -g so that it will override -g1 but not -g3
This patch partially reverts a change in how a bare -g option was parsed in a previous commit. Originally, -g would set the debug level to 2 only if debug was off. The previous commit changed that so that -g would set the debug level to 2 unconditionally. This patch changes it so that -g sets the debug level to 2 if it was either off or at level 1 before. OK to commit? -cary 2014-05-14 Cary Coutant gcc/ PR debug/61013 * opts.c (common_handle_option): Don't special-case "-g". (set_debug_level): Default to at least level 2 with "-g". Index: gcc/opts.c === --- gcc/opts.c (revision 210437) +++ gcc/opts.c (working copy) @@ -1814,13 +1814,8 @@ common_handle_option (struct gcc_options break; case OPT_g: - /* -g by itself should force -g2. */ - if (*arg == '\0') - set_debug_level (NO_DEBUG, DEFAULT_GDB_EXTENSIONS, "2", opts, opts_set, -loc); - else - set_debug_level (NO_DEBUG, DEFAULT_GDB_EXTENSIONS, arg, opts, opts_set, -loc); + set_debug_level (NO_DEBUG, DEFAULT_GDB_EXTENSIONS, arg, opts, opts_set, + loc); break; case OPT_gcoff: @@ -2070,10 +2065,12 @@ set_debug_level (enum debug_info_type ty opts_set->x_write_symbols = type; } - /* A debug flag without a level defaults to level 2. */ + /* A debug flag without a level defaults to level 2. + If off or at level 1, set it to level 2, but if already + at level 3, don't lower it. */ if (*arg == '\0') { - if (!opts->x_debug_info_level) + if (opts->x_debug_info_level < DINFO_LEVEL_NORMAL) opts->x_debug_info_level = DINFO_LEVEL_NORMAL; } else
Re: [GOOGLE] Updates highest_location when updating next_discriminator_location
> Attached patch passes regression tests and benchmark test. OK for google-4_9? OK. Thanks again! -cary
[google/gcc-4_9] Force the use of -ggnu-pubnames when using -gsplit-dwarf
I've backported this patch from trunk at r210395. -cary gcc/ * opts.c (finish_options): Use -ggnu-pubnames with -gsplit-dwarf.
[PATCH] Use -ggnu-pubnames with -gsplit-dwarf
This patch forces the use of -ggnu-pubnames when using -gsplit-dwarf. This is necessary so that the gold linker can generate .gdb_index version 7. No new regressions. Committed as trivial (has no effect if you're not using -gsplit-dwarf). -cary 2014-05-13 Cary Coutant gcc/ * opts.c (finish_options): Use -ggnu-pubnames with -gsplit-dwarf. Index: opts.c === --- opts.c (revision 210389) +++ opts.c (working copy) @@ -857,9 +857,9 @@ finish_options (struct gcc_options *opts maybe_set_param_value (PARAM_MAX_STORES_TO_SINK, 0, opts->x_param_values, opts_set->x_param_values); - /* The -gsplit-dwarf option requires -gpubnames. */ + /* The -gsplit-dwarf option requires -ggnu-pubnames. */ if (opts->x_dwarf_split_debug_info) -opts->x_debug_generate_pub_sections = 1; +opts->x_debug_generate_pub_sections = 2; } #define LEFT_COLUMN27
Re: [GOOGLE] Updates highest_location when updating next_discriminator_location
> The problem is that linemap_location_from_macro_expansion_p will > always return true if locus has discriminator. And in linemap_lookup, > this will lead to call linemap_macro_map_lookup, in which there is an > assertion: > > linemap_assert (line >= LINEMAPS_MACRO_LOWEST_LOCATION (set)); > > However, line is actually not a macro location. That sounds like we're leaking a discriminator location into the linemap code. Before you can call linemap_location_from_macro_expansion_p, you need to do this (as in expand_location_1): /* If LOC describes a location with a discriminator, extract the discriminator and map it to the real location. */ if (min_discriminator_location != UNKNOWN_LOCATION && loc >= min_discriminator_location && loc < next_discriminator_location) loc = map_discriminator_location (loc); -cary
Re: [GOOGLE] Updates highest_location when updating next_discriminator_location
> Index: gcc/input.c > === > --- gcc/input.c (revision 210338) > +++ gcc/input.c (working copy) > @@ -910,6 +910,8 @@ location_with_discriminator (location_t locus, int >: next_discriminator_location); > >next_discriminator_location++; > + if (next_discriminator_location > line_table->highest_location) > +line_table->highest_location = next_discriminator_location; >return ret; > } It seems to me that if something is breaking because highest_location isn't getting updated, there's a deeper problem. The discriminator handling depends on the fact that the line table doesn't ever add any new values after min_discriminator_location is set, and the routines in libcpp shouldn't ever see a location_t value above min_discriminator_location (it should be converted to a "real" location_t via map_discriminator_location()). If libcpp now assigns new location_t values after we start handing out discriminators, then we'll probably need to have libcpp start handling discriminator values (which is the solution for LTO as well). If you're just seeing leakage of discriminator locations into libcpp, there's probably a spot where we need to call map_discriminator_location(). -cary
[google/gcc-4_8] Add -fskeleton-type-units flag.
Add -f[no-]skeleton-type-units flag to control whether GCC generates skeleton type units. This patch is for the google/gcc-4_8 branch. I will hold off sending it upstream until we're certain that we will be disabling skeleton type units, at which point, I'll just remove the code instead of making it conditional on this flag. OK to commit? -cary 2014-05-12 Cary Coutant gcc/ * dwarf2out.c (output_skeleton_debug_sections): Check -fskeleton-type-units flag. (output_comdat_type_unit): Likewise. (dwarf2out_finish): Likewise. * common.opt (fskeleton-type-units): New option. Index: dwarf2out.c === --- dwarf2out.c (revision 208284) +++ dwarf2out.c (working copy) @@ -8978,7 +8978,7 @@ 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) + if (use_debug_types && flag_skeleton_type_units) output_die_abbrevs (SKELETON_TYPE_DIE_ABBREV, get_skeleton_type_unit ()); dw2_asm_output_data (1, 0, "end of skeleton .debug_abbrev"); @@ -9043,7 +9043,7 @@ output_comdat_type_unit (comdat_type_nod unmark_dies (node->root_die); #if defined (OBJECT_FORMAT_ELF) - if (dwarf_split_debug_info) + if (dwarf_split_debug_info && flag_skeleton_type_units) { /* Produce the skeleton type-unit header. */ const char *secname = ".debug_types"; @@ -23930,7 +23930,8 @@ dwarf2out_finish (const char *filename) add_top_level_skeleton_die_attrs (main_comp_unit_die); add_AT_lineptr (main_comp_unit_die, DW_AT_GNU_addr_base, debug_addr_section_label); - (void) get_skeleton_type_unit (); + if (flag_skeleton_type_units) +(void) get_skeleton_type_unit (); htab_traverse_noresize (debug_str_hash, index_string, &index); } Index: common.opt === --- common.opt (revision 208284) +++ common.opt (working copy) @@ -1348,6 +1348,10 @@ floop-nest-optimize Common Report Var(flag_loop_optimize_isl) Optimization Enable the ISL based loop nest optimizer +fskeleton-type-units +Common Report Var(flag_skeleton_type_units) Init(1) +Output skeleton .debug_types sections when using -gsplit-dwarf. + fstrict-volatile-bitfields Common Report Var(flag_strict_volatile_bitfields) Init(-1) Force bitfield accesses to match their type width
Re: [GOOGLE] backport discriminator support from google-4_8 to google-4_9
> Yes, this patch is a combination of all these patches. Some of them > are already in trunk. OK for google 4.9 branch. Thanks! -cary
Re: [GOOGLE] backport discriminator support from google-4_8 to google-4_9
On Mon, May 12, 2014 at 1:11 PM, Dehao Chen wrote: > This patch backports r199154 from google-4_8 to google-4_9 > > Bootstrapped and passed regression test. > > OK for google-4_9 branch? Don't forget the follow-ons listed below. Any reason not to combine them into this patch? Looks good. Thanks! -cary r201928 | dehao | 2013-08-22 10:20:29 -0700 (Thu, 22 Aug 2013) | 7 lines Set discriminator for call stmts within a same basic block. 2013-08-22 Dehao Chen * gcc/tree-cfg.c (assign_discriminators): assign discriminator for call stmt in a same BB if it is mapped to a same line. r201857 | dehao | 2013-08-19 14:26:33 -0700 (Mon, 19 Aug 2013) | 7 lines Fix the discriminator assignment bug during hashing. 2013-08-19 Dehao Chen * tree-cfg.c (next_discriminator_for_locus): Fix discriminator assignment bug. r200655 | dehao | 2013-07-03 18:15:18 -0700 (Wed, 03 Jul 2013) | 10 lines Updates discriminator in a correct way. 2013-07-03 Dehao Chen * gcc/input.c (location_with_discriminator): Updates discrminator in a correct way. (get_discriminator_from_locus): Change data type. * gcc/tree-cfg.c (same_line_p): Use expand_location to check same_line. (assign_discriminator): Updates discrminator in a correct way. r199303 | dehao | 2013-05-24 10:04:31 -0700 (Fri, 24 May 2013) | 20 lines Backport r199295 from trunk. Change the discriminator assignment algorithm to make it more robust. 2013-05-24 Dehao Chen * gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c: New Testcase. * gcc/tree-cfg.c (locus_descrim_hasher::hash): Change discrminator hash function. (locus_descrim_hasher::equal): Likewise. (build_gimple_cfg): New discrminator assignmnet algorithm (make_edges): Likewise. (next_discriminator_for_locus): Likewise. (same_line_p): Likewise. (assign_discriminators): Likewise. (make_cond_expr_edges): Likewise. (make_gimple_switch_edges): Likewise. (make_goto_expr_edges): Likewise. (make_gimple_asm_edges): Likewise.
[google/gcc-4_9] PR debug/60929: Fix a few ICEs and other problems with -fdebug-types-sections
I've backported the following patch from trunk at r209812. Committed on the google/gcc-4_9 branch at r209875. Google ref: 14230806. -cary gcc/ * dwarf2out.c (should_move_die_to_comdat): A type definition can contain a subprogram definition, but don't move it to a comdat unit. (clone_as_declaration): Copy DW_AT_abstract_origin attribute. (generate_skeleton_bottom_up): Remove DW_AT_object_pointer attribute from original DIE. (clone_tree_hash): Rename to... (clone_tree_partial): ...this; change callers. Copy DW_TAG_subprogram DIEs as declarations. (copy_decls_walk): Don't copy children of a declaration into a type unit. gcc/testsuite/ * g++.dg/debug/dwarf2/dwarf4-nested.C: New test case. * g++.dg/debug/dwarf2/dwarf4-typedef.C: Add -fdebug-types-section flag.
Re: PR debug/60929: Fix a few ICEs and other problems with -fdebug-types-sections
What are the rules for backporting to 4.9.1? Should I backport this patch? -cary > 2014-04-25 Cary Coutant > > gcc/ > PR debug/60929 > * dwarf2out.c (should_move_die_to_comdat): A type definition > can contain a subprogram definition, but don't move it to a > comdat unit. > (clone_as_declaration): Copy DW_AT_abstract_origin attribute. > (generate_skeleton_bottom_up): Remove DW_AT_object_pointer attribute > from original DIE. > (clone_tree_hash): Rename to... > (clone_tree_partial): ...this; change callers. Copy > DW_TAG_subprogram DIEs as declarations. > (copy_decls_walk): Don't copy children of a declaration into a > type unit. > > gcc/testsuite/ > PR debug/60929 > * g++.dg/debug/dwarf2/dwarf4-nested.C: New test case. > * g++.dg/debug/dwarf2/dwarf4-typedef.C: Add -fdebug-types-section > flag.
PR debug/60929: Fix a few ICEs and other problems with -fdebug-types-sections
Fix a few ICEs and other problems with -fdebug-types-sections. This is a followup to an earlier proposed patch: http://gcc.gnu.org/ml/gcc-patches/2012-08/msg01998.html http://gcc.gnu.org/ml/gcc-patches/2012-09/msg0.html (1) If a function contains a local typedef of an anonymous structure, GCC will generate a typedef DIE in the function, where the typedef DIE points to a structure_type DIE at the top level. That structure_type DIE, if it's a non-POD, can contain ctor and dtor definitions. This causes an assertion in should_move_die_to_comdat to fail, as we have up to now assumed that this could never happen. (2) With --std=c++11, a template parameter can refer to a local type defined within a function. Because that local type doesn't qualify for its own type unit, we copy it as an "unworthy" type into the type unit that refers to it, but we copy too much, leading to a comdat type unit that contains a DIE with subprogram definitions rather than declarations. These DIEs may have DW_AT_low_pc/high_pc or DW_AT_ranges attributes, and consequently can refer to range list entries that don't get emitted because they're not marked when the compile unit is scanned, eventually causing an undefined symbol at link time. (3) When a class template instantiation is moved into a separate type unit, it can bring along a lot of other referenced types into the type unit, especially if the template is derived from another (large) type that does not have an actually have a type definition in a type unit of its own. When there are many instantiations of the same template, we get a lot of duplication, and in the worst case (a template with several parameters, instantiated multiple times along each dimension), GCC can end up taking a long time and exhausting available memory. This combinatorial explosion is being caused by copy_decls_walk, where it finds a type DIE that is referenced by the type unit, but is not itself a type unit, and copies a declaration for that type into the type unit in order to resolve the reference within the type unit. In the process, copy_decls_walk also copies all of the children of that DIE. In the case of a base class with member function templates, every one of the instantiated member functions is copied into every type unit that references the base class. Bootstrapped and regression tested on x86_64. Committed as r209812. -cary 2014-04-25 Cary Coutant gcc/ PR debug/60929 * dwarf2out.c (should_move_die_to_comdat): A type definition can contain a subprogram definition, but don't move it to a comdat unit. (clone_as_declaration): Copy DW_AT_abstract_origin attribute. (generate_skeleton_bottom_up): Remove DW_AT_object_pointer attribute from original DIE. (clone_tree_hash): Rename to... (clone_tree_partial): ...this; change callers. Copy DW_TAG_subprogram DIEs as declarations. (copy_decls_walk): Don't copy children of a declaration into a type unit. gcc/testsuite/ PR debug/60929 * g++.dg/debug/dwarf2/dwarf4-nested.C: New test case. * g++.dg/debug/dwarf2/dwarf4-typedef.C: Add -fdebug-types-section flag. Index: gcc/dwarf2out.c === --- gcc/dwarf2out.c (revision 209731) +++ gcc/dwarf2out.c (working copy) @@ -6830,14 +6830,13 @@ should_move_die_to_comdat (dw_die_ref di case DW_TAG_structure_type: case DW_TAG_enumeration_type: case DW_TAG_union_type: - /* Don't move declarations, inlined instances, or types nested in a -subprogram. */ + /* Don't move declarations, inlined instances, types nested in a +subprogram, or types that contain subprogram definitions. */ if (is_declaration_die (die) || get_AT (die, DW_AT_abstract_origin) - || is_nested_in_subprogram (die)) + || is_nested_in_subprogram (die) + || contains_subprogram_definition (die)) return 0; - /* A type definition should never contain a subprogram definition. */ - gcc_assert (!contains_subprogram_definition (die)); return 1; case DW_TAG_array_type: case DW_TAG_interface_type: @@ -6926,6 +6925,7 @@ clone_as_declaration (dw_die_ref die) switch (a->dw_attr) { +case DW_AT_abstract_origin: case DW_AT_artificial: case DW_AT_containing_type: case DW_AT_external: @@ -7158,6 +7158,12 @@ generate_skeleton_bottom_up (skeleton_ch dw_die_ref clone = clone_die (c); move_all_children (c, clone); + /* If the original has a DW_AT_object_pointer attribute, + it would now point to a child DIE just moved to the + cloned tree, so we need to remove that attribute from + the original. */ + remove_AT (c, DW_AT_object_
Re: [PATCH] dwarf2out: Use normal constant values in bound_info if possible.
> + /* If HOST_WIDE_INT is big enough then represent the bound as > + a constant value. Note that we need to make sure the type > + is signed or unsigned. We cannot just add an unsigned > + constant if the value itself is positive. Some DWARF > + consumers will lookup the bounds type and then sign extend > + any unsigned values found for signed types. This is only > + for DW_AT_lower_bound, normally unsigned values > + (DW_FORM_data[1248]) are assumed to not need > + sign-extension. */ This comment confuses me. By "we need to make sure the type is signed or unsigned" (what else can it be?), I think you mean "we need to choose a form based on whether the type is signed or unsigned." And by "This is only for DW_AT_lower_bound, ...", I think you mean "This is needed only for DW_AT_{lower,upper}_bound, since for most other attributes, consumers will treat DW_FORM_data[1248] as unsigned values, regardless of the underlying type." Otherwise, the patch looks OK to me. -cary
Re: [PATCH] Add DW_AT_const_value as unsigned or int depending on type and value used.
> Added a clarifying comment to the code and reinstated the TODO for the > double case. OK to push? > > * dwarf2out.c (gen_enumeration_type_die): Add DW_AT_const_value > as unsigned or int depending on type and value used. OK. Thanks! -cary
Re: [PATCH] Add DW_AT_const_value as unsigned or int depending on type and value used.
>> Also note that size_of_die and value_format will still choose >> DW_FORM_data[1248] for dw_val_class_unsigned_const in most cases. >> Don't you really want to use DW_FORM_udata? > > DW_FORM_data[1248] is in many cases smaller than DW_FORM_udata (though, one > has to take into account possibly larger .debug_abbrev size). Yes, but it's up to the consumer to deduce from context whether the value is signed or unsigned. If it's still true that GDB will interpret DW_FORM_data[1248] as signed (as the deleted comment said), and you output a value between 128 and 255 using DW_FORM_data1, this isn't going to work. Maybe that comment only applies to DW_FORM_data[48] (whichever matches HOST_WIDE_INT)? At the very least, you should replace the comment block you removed with one that explains why GDB will interpret the values correctly. Keep in mind that the DWARF standard "strongly" encourages producers to use DW_FORM_udata and DW_FORM_sdata. -cary
Re: [PATCH] Add DW_AT_const_value as unsigned or int depending on type and value used.
>>* dwarf2out.c (gen_enumeration_type_die): Add DW_AT_const_value >>as unsigned or int depending on type and value used. > > Since stage 1 opened up I would like to request approval again to push > this. Patch rebased to current master attached. The discussion that led to that TODO is here: http://gcc.gnu.org/ml/gcc-patches/2013-04/msg01086.html In that thread, I suggested a reorganization of this piece of code that would also take care of the wider-than-unsigned-hwi case. Also note that size_of_die and value_format will still choose DW_FORM_data[1248] for dw_val_class_unsigned_const in most cases. Don't you really want to use DW_FORM_udata? -cary
Re: [PATCH] PR debug/16063. Add DW_AT_type to DW_TAG_enumeration.
>> The DWARF bits are fine with me. > > Thanks. Who can approve the other bits? You should probably get C and C++ front end approval. I'm not really sure who needs to review patches in c-family/. Since the part in c/ is so tiny, maybe all you need is a C++ front end maintainer. Both Richard Henderson and Jason Merrill are global reviewers, so either of them could approve the whole thing. > When approved should I wait till stage 1 opens before committing? Yes. The PR you're fixing is an enhancement request, not a regression, so it needs to wait. -cary
Re: [PATCH] PR debug/16063. Add DW_AT_type to DW_TAG_enumeration.
> However it would be nice to be assured that the gcc change is ok in > principle first. The DWARF bits are fine with me. -cary
Re: [PATCH] PR debug/57519 - Emit DW_TAG_imported_declaration under the right class for 'using' statements in a class
> ChangeLog: > 2014-03-25 Siva Chandra Reddy > > Fix PR debug/57519 > > /cp > > PR debug/57519 > * class.c (handle_using_decl): Pass the correct scope to > cp_emit_debug_info_for_using. > > testsuite/ > > PR debug/57519 > * g++.dg/debug/dwarf2/imported-decl-2.C: New testcase. This looks right to me, but you'll need approval from a C++ front end maintainer. Thanks! -cary
Re: [PATCH] PR debug/16063. Add DW_AT_type to DW_TAG_enumeration.
> gcc/cp/ > * dwarf2out.c (gen_enumeration_type_die): Add DW_AT_type if > enum_underlying_base_type defined and DWARF version > 3. > * langhooks.h (struct lang_hooks_for_types): Add > enum_underlying_base_type. > * langhooks-def.h (LANG_HOOKS_ENUM_UNDERLYING_BASE_TYPE): New define. > (LANG_HOOKS_FOR_TYPES_INITIALIZER): Add new lang hook. This should be the ChangeLog entry for cp/cp-lang.c. -cary
[google/gcc-4_8] Fix demangler to handle conversion operators correctly
I've backported this patch from trunk at r205292. Committed on google/gcc-4_8 branch as r207971. Tested with make check in libiberty. -cary 2013-11-22 Cary Coutant libiberty/ * 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.
Re: [PATCH] Don't add DW_AT_{main_subprogram,calling_convention} attributes more than once (PR debug/60152)
> gen_subprogram_die is often called more than once for the same decl > (e.g. the first time through force_decl_die etc.), but it always > unconditionally calls add_calling_convention_attribute which thus > may add the attributes several times. > > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for > trunk? Looks good to me. > Alternatively, we could avoid the add_calling_convention_attribute > call in gen_subprogram_die if subr_die == orig_die, i.e. if we have > already processed the decl once, hopefully the calling conventions would be > the same in all cases. s/orig_die/old_die/ This sounds a little more efficient -- no calls to get_AT just to see if the attribute has already been added. I've got a slight preference for your alternate proposal (assuming it works). -cary
[google gcc-4_8] Remove DW_AT_GNU_addr_base from skeleton type unit DIEs
Remove DW_AT_GNU_addr_base from skeleton type unit DIEs. GCC currently adds a DW_AT_GNU_addr_base attribute to skeleton type unit DIEs, even though it's not needed there. By removing it, we can save the 8 bytes that a DW_FORM_addr takes up, plus the 24 bytes that the corresponding relocation takes. This patch is for the google/gcc-4_8 branch. I will submit it for trunk when stage 1 is open. Tested with crosstool_validate.py. 2014-02-11 Cary Coutant * dwarf2out.c (add_top_level_skeleton_die_attrs): Don't add DW_AT_GNU_addr_base to all skeleton DIEs. (dwarf2out_finish): Add it only to the skeleton comp_unit DIE. Index: dwarf2out.c === --- dwarf2out.c (revision 207671) +++ dwarf2out.c (working copy) @@ -8918,7 +8918,6 @@ add_top_level_skeleton_die_attrs (dw_die if (comp_dir != NULL) add_skeleton_AT_string (die, DW_AT_comp_dir, comp_dir); add_AT_pubnames (die); - add_AT_lineptr (die, DW_AT_GNU_addr_base, debug_addr_section_label); } /* Return the single type-unit die for skeleton type units. */ @@ -23929,6 +23928,8 @@ 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); + add_AT_lineptr (main_comp_unit_die, DW_AT_GNU_addr_base, + debug_addr_section_label); (void) get_skeleton_type_unit (); htab_traverse_noresize (debug_str_hash, index_string, &index); }
Re: [C++ RFC/Patch] PR 58561
> in this bug we ICE in dwarf2out.c:is_base_type with -g, because it doesn't > know how to handle the TEMPLATE_TYPE_PARM coming from the C++ front-end and > representing 'auto' in this kind of C++1y code. > > That it shouldn't ICE and return 0 instead I'm pretty sure, I'm less sure > about the next problem, that is what gen_type_die_with_usage should do: > build a DW_TAG_unspecified_type, like happens for NULLPTR_TYPE or > LANG_TYPE?!? Anyway, my wild guessing resulted in adding an hook, per the > below draft (in any case, tentative names and comments but passes testing). > Or we want something completely different? We've been discussing this in the DWARF workgroup, and the current proposal on the table is to use DW_TAG_unspecified_type, as you have done here. This looks OK to me (although I kind of wish we had a better way of testing for "auto" than doing a string compare!). Depending on further discussions in the workgroup, we'll probably need to do a bit more work to support auto return types, but I think this is the right direction. -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] 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: [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: [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: [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
> 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: [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
>> 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: [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