Re: RFA/RFC: Add stack recursion limit to libiberty's demangler

[Cary Coutant]

> 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

[Cary Coutant]

> 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

[Cary Coutant]

> 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

[Cary Coutant]

> 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

[Cary Coutant]

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

[Cary Coutant]

>> 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

[Cary Coutant]

> 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

[Cary Coutant]

> 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

[Cary Coutant]

> 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

[Cary Coutant]

>> 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

[Cary Coutant]

> 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

[Cary Coutant]

>   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

[Cary Coutant]

> 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=open .  */
DW_OP (DW_OP_GNU_implicit_pointer, 0xf2)
/* The GNU entry value extension.
   See http://www.dwarfstd.org/ShowIssue.php?issue=100909.1=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

[Cary Coutant]

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

[Cary Coutant]

I'm committing this patch to sync the top-level configure with binutils-gdb.

-cary


2016-03-17  Cary Coutant  <ccout...@gmail.com>

* 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

[Cary Coutant]

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

[Cary Coutant]

 Based on Richard's suggestion, I have a patch to localize comdat
 functions which seems like a very effective solution to this problem.
 The text size increase is limited to the extra comdat copies generated
 for the specialized modules (modules with unsafe options) which is
 usually only a few.   Since -fweak does something similar for
 functions,  I have called the new option -fweak-comdat-functions.
 This does not apply to virtual comdat functions as their addresses can
 always be leaked via the vtable. Using this flag with virtual
 functions generates a warning.

+fweak-comdat-functions
+C++ Var(flag_weak_comdat_functions) Init(1)
+Specific to comdat functions(-fno-weak-comdat-functions : Localize
Comdat Functions).
+With -fno-weak-comdat-functions, virtual comdat functions are still linked as
+weak functions.  With -fno-weak-comdat-functions, the address of the comdat
+functions that are localized will be unique and this can cause unintended
+behavior when addresses of comdat functions are used.

Is one of those With -fno-weak-comdat-functions supposed to be With
-fweak-comdat-functions? This description doesn't really say what the
flag (without the no) does, and doesn't explain what localize
means.

+@item -fno-weak-comdat-functions
+@opindex fno-weak-comdat-functions
+Do not use weak symbol support for comdat non-virtual functions, even if it
+is provided by the linker.  By default, G++ uses weak symbols if they are
+available.  This option is useful when comdat functions generated in certain
+compilation units need to be kept local to the respective units and not exposed
+globally.  This does not apply to virtual comdat functions as their pointers
+may be taken via virtual tables.  This can cause unintended behavior if
+the addresses of comdat functions are used.

It's not really the weak that is causing the problem -- it's the
comdat. What the option really is doing is making the functions
static rather than comdat. (It's all gated under flag_weak because
weak symbols are the fall-back to link-once and comdat symbols.) I'd
suggest phrasing this more in terms of static vs. comdat.

This looks like the right way to go, though.

-cary

Re: [RFC] COMDAT Safe Module Level Multi versioning

[Cary Coutant]

 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

[Cary Coutant]

 +@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: [PATCH] Add extensions to dwarf2.def

[Cary Coutant]

 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

[Cary Coutant]

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  ccout...@gmail.com

* MAINTAINERS: Update my email address.


Index: MAINTAINERS
===
--- MAINTAINERS (revision 221926)
+++ MAINTAINERS (working copy)
@@ -195,7 +195,7 @@ caller-save.c   Jeff Law
 l...@redhat.com
 callgraph  Jan Hubicka hubi...@ucw.cz
 debugging code Jim Wilson  wil...@tuliptree.org
 dwarf debugging code   Jason Merrill   ja...@redhat.com
-dwarf debugging code   Cary Coutantccout...@google.com
+dwarf debugging code   Cary Coutantccout...@gmail.com
 c++ runtime libs   Paolo Carlini   paolo.carl...@oracle.com
 c++ runtime libs   Ulrich Drepper  drep...@gmail.com
 c++ runtime libs   Benjamin De Kosnik  b...@gnu.org
@@ -300,7 +300,7 @@ libsanitizer, asan.cDmitry Vyukov   dvy
 loop optimizer Zdenek Dvorak   o...@ucw.cz
 loop optimizer Daniel Berlin   dber...@dberlin.org
 LTORichard Biener  rguent...@suse.de
-LTO plugin Cary Coutantccout...@google.com
+LTO plugin Cary Coutantccout...@gmail.com
 Plugin Le-Chun Wu  l...@google.com
 register allocationPeter Bergner   berg...@vnet.ibm.com
 register allocationKenneth Zadeck  zad...@naturalbridge.com
@@ -365,7 +365,7 @@ William Cohen
 wco...@redhat.com
 Josh Connerjcon...@apple.com
 R. Kelley Cook kc...@gcc.gnu.org
 Christian Cornelssen   cc...@cs.tu-berlin.de
-Cary Coutant   ccout...@google.com
+Cary Coutant   ccout...@gmail.com
 Lawrence Crowl cr...@google.com
 Ian Dall   i...@beware.dropbear.id.au
 David Daneydavid.da...@caviumnetworks.com

Re: [google/gcc-4_9] Minor changes to -ftwo-level-line-tables

[Cary Coutant]

 @@ -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}

[Cary Coutant]

 DW_LANG_Fortran03 and DW_LANG_Fortran08 DW_AT_language values were recently
 accepted into DWARF5.  This patch changes GCC to handle those similarly to
 how e.g. the -std=c++11, -std=c++14 or -std=c11 are handled.

 As it will take some time for consumers to catch up, I'm enabling that
 only if -gdwarf-5 is used for now.

My concern with enabling -gdwarf-5 at this point is that all we're
really doing with it is enabling a subset of DWARF-5 features (as we
did with -gdwarf-4). We're still putting a version number of 2 in the
compilation unit header! But I guess even upgrading the CU header to
version 3 is something not all consumers are yet ready for. As long as
we selectively enable DWARF-5 features while still claiming to be
DWARF-2, I guess we're OK, but how will we decide to upgrade fully
beyond DWARF-2, and what option will we use for that?

The DWARF bits of this patch are OK with me.

-cary

Re: [RFC PATCH] Emit DW_LANG_Fortran{03,08}

[Cary Coutant]

 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}

[Cary Coutant]

 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: PR lto/64837: lto plugin doesn't call ld_plugin_release_input_file

[Cary Coutant]

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

[Cary Coutant]

Ping^3. Should I be addressing this to someone else?

-cary

On Mon, Dec 1, 2014 at 2:15 PM, Cary Coutant ccout...@google.com wrote:
 Ping^2.

 -cary

 On Wed, Oct 29, 2014 at 12:04 PM, Cary Coutant ccout...@google.com wrote:
 Ping?

 On Mon, Oct 20, 2014 at 10:31 AM, Cary Coutant ccout...@google.com wrote:
 This patch adds support for the mips target in gold.

 OK to commit?

 -cary


 2014-10-20  Cary Coutant  ccout...@google.com

 * 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

[Cary Coutant]

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 hjl.to...@gmail.com wrote:
 On Wed, Jan 28, 2015 at 11:37 AM, H.J. Lu hjl.to...@gmail.com wrote:
 On Wed, Jan 28, 2015 at 11:19 AM, Richard Biener
 richard.guent...@gmail.com wrote:
 On January 28, 2015 7:12:43 PM CET, H.J. Lu hongjiu...@intel.com 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  hongjiu...@intel.com

 + * lto-plugin.c (claim_file_handler): Call release_input_file only
 + if it is not NULL.
 +
 +2015-01-28  H.J. Lu  hongjiu...@intel.com
 +
   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

[Cary Coutant]

 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

[Cary Coutant]

 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

[Cary Coutant]

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

[Cary Coutant]

  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

[Cary Coutant]

 +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 block_entry
 +{
 +  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_tableblock_hasher *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

[Cary Coutant]

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] DWARF add DW_AT_noreturn on noreturn function subprogram.

[Cary Coutant]

 Presumably we don't have any sense when the values will be assigned, right?
 Any chance we could speed that along a bit?

As Jason said, the value in the current draft is unlikely to change,
and I think he and I can probably lobby to keep it unchanged if there
any danger that the numbering will change. The committee doesn't
really like it when we jump the gun and use values before the final
spec is published, but as a practical matter, it's often necessary and
(at this stage) pretty safe.

-cary

Re: [PATCH] DWARF add DW_AT_noreturn on noreturn function subprogram.

[Cary Coutant]

[+cc Michael Eager]

 Rather than having to lobby to keep it unchanged because we jumped the gun,
 can we lobby to get the number assigned in the near future rather than in
 the potentially far future?  That feels more cooperative to me :-)

 Would that make Michael happier?

I'm pretty confident that Michael will say, don't rely on the value
until the final spec is published. (He's told me exactly that in the
past. I've been guilty of jumping the gun on a couple of DW_LANG codes
and a few DW_AT values.)

But we should let Michael answer for himself...

Michael, for your reference, here's the start of this thread:

   https://gcc.gnu.org/ml/gcc-patches/2014-11/msg03195.html

and its continuation into December:

   https://gcc.gnu.org/ml/gcc-patches/2014-12/msg00099.html

Michael, are you OK with sharing your target dates for publishing the spec?

-cary

Re: [PATCH] Add top-level config support for gold mips target

[Cary Coutant]

Ping^2.

-cary

On Wed, Oct 29, 2014 at 12:04 PM, Cary Coutant ccout...@google.com wrote:
 Ping?

 On Mon, Oct 20, 2014 at 10:31 AM, Cary Coutant ccout...@google.com wrote:
 This patch adds support for the mips target in gold.

 OK to commit?

 -cary


 2014-10-20  Cary Coutant  ccout...@google.com

 * configure (--enable-gold): Add mips*-*-*.
 * configure.ac: Regenerate.


 Index: configure
 ===
 --- configure   (revision 216487)
 +++ configure   (working copy)
 @@ -2941,7 +2941,7 @@ case ${ENABLE_GOLD} in
# Check for target supported by gold.
case ${target} in
  i?86-*-* | x86_64-*-* | sparc*-*-* | powerpc*-*-* | arm*-*-* \
 -| aarch64*-*-* | tilegx*-*-*)
 +| aarch64*-*-* | tilegx*-*-* | mips*-*-*)
   configdirs=$configdirs gold
   if test x${ENABLE_GOLD} = xdefault; then
 default_ld=gold
 Index: configure.ac
 ===
 --- configure.ac(revision 216487)
 +++ configure.ac(working copy)
 @@ -332,7 +332,7 @@ case ${ENABLE_GOLD} in
# Check for target supported by gold.
case ${target} in
  i?86-*-* | x86_64-*-* | sparc*-*-* | powerpc*-*-* | arm*-*-* \
 -| aarch64*-*-* | tilegx*-*-*)
 +| aarch64*-*-* | tilegx*-*-* | mips*-*-*)
   configdirs=$configdirs gold
   if test x${ENABLE_GOLD} = xdefault; then
 default_ld=gold

Re: [PATCH 2/3] PR other/61321 - demangler crash on casts in template parameters

[Cary Coutant]

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 ccout...@google.com 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 pal...@redhat.com wrote:
 On 07/24/2014 11:35 PM, Cary Coutant wrote:
 It seems that the problem here is more general; a template argument list is
 not in scope within that same template argument list.  Can't we fix that
 without special-casing conversion ops?

 I think conversion ops really are a special case.

 Thanks Cary.  FWIW, I agree.

 (GDB 7.8 hasn't been released yet, though it's close.  If this
 patch is approved as is, we'll be able to have the crash
 fixed there.  If this requires a significant rewrite though,
 I'm afraid I might not be able to do it myself anytime soon.)

 It's the only case
 where the template parameters refer to the template argument list from
 the cast operator's enclosing template. In a cast expression, like
 anywhere else you might have a template parameter, the template
 parameter refers to the template argument list of the immediately
 enclosing template.

 I think this note from Section 5.1.3 (Operator Encodings) of the ABI
 is what makes this a special case (it's an informative comment in the
 document, but seems to me to be normative):

 For a user-defined conversion operator the result type (i.e., the
 type to which the operator converts) is part of the mangled name of
 the function. If the conversion operator is a member template, the
 result type will appear before the template parameters. There may be
 forward references in the result type to the template parameters.


 --
 Thanks,
 Pedro Alves

[google/gcc-4_9] Backport pending patch to fix demangler crash

[Cary Coutant]

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 pal...@redhat.com

include/
* demangle.h (enum demangle_component_type)
DEMANGLE_COMPONENT_CONVERSION: New value.

2014-05-27  Pedro Alves pal...@redhat.com

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 );
- 

Re: [PATCH] Add top-level config support for gold mips target

[Cary Coutant]

Ping?

On Mon, Oct 20, 2014 at 10:31 AM, Cary Coutant ccout...@google.com wrote:
 This patch adds support for the mips target in gold.

 OK to commit?

 -cary


 2014-10-20  Cary Coutant  ccout...@google.com

 * 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

[Cary Coutant]

This patch adds support for the mips target in gold.

OK to commit?

-cary


2014-10-20  Cary Coutant  ccout...@google.com

* 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

[Cary Coutant]

 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

[Cary Coutant]

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 pal...@redhat.com 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

[Cary Coutant]

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 ja...@redhat.com wrote:
 On Fri, Oct 10, 2014 at 12:07:03AM +0400, Ilya Verbin wrote:
 On 09 Oct 16:07, Ilya Verbin wrote:
+  /* By default linker does not discard .gnu.offload_lto_* 
sections.  */
+  const char *linker_script = make_temp_file (_linker_script.x);
+  FILE *stream = fopen (linker_script, w);
+  if (!stream)
+   fatal_error (fopen %s: %m, linker_script);
+  fprintf (stream, SECTIONS { /DISCARD/ : { *(
+  OFFLOAD_SECTION_NAME_PREFIX *) } }\n);
+  fclose (stream);
+  printf (%s\n, linker_script);
+
+  goto finish;
+}
  
   Does this work with gold?  Are there any other linkers that support 
   plugins,
   but don't support linker scripts this way?
 
  Oops, gold does not support scripts, outputted from plugins :(
  error: SECTIONS seen after other input files; try -T/--script
 
  Probably, we should update default linker scripts in binutils?
  But without latest ld/gold all binaries compiled without -flto and with
  offloading will contain intermediate bytecode...

 Actually, this issue is not due to outputting a script from a plugin,
 gold just does not support partial linker scripts:
 https://sourceware.org/bugzilla/show_bug.cgi?id=17451

 So it seems that discarding .gnu.offload_lto_* sections (like it is done for
 .gnu.lto_*) in the default ld and gold scripts is the right way?

 I must say I'm not very much familiar with the linker plugin API, but it
 surprises me that discarding sections is not something it allows.
 Anyway, can you do the partial linker script for the bfd linker (is there
 a way to determine from the linker plugin API if it is gold or bfd ld?), and
 for gold for the time being perhaps strip the sections in lto-wrapper? and
 feed the ET_REL objects with the sections stripped back to the linker
 through the plugin API?

 Jakub

Re: [PATCH 4/n] OpenMP 4.0 offloading infrastructure: lto-wrapper

[Cary Coutant]

 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: [google/gcc-4_9] Add gcc driver option -no-pie

[Cary Coutant]

 If adding a new option, you need to document it in invoke.texi.

 Patch updated.

 Is this alright for google/gcc-4_9?

+no-pie
+Driver RejectNegative Negative(pie)
+Create a position dependent executable

I'd suggest adding an alias for -no-pie (meaning --no-pie) -- see
earlier in common.opt where -pie is declared as an alias for pie,
and similarly for -shared.

I wonder about the spelling -- should it be -no-pie or -nopie? GCC
options seem to favor no options without a hyphen, but it's not very
consistent, so it's probably good the way you've spelled it -- it
better matches the way the linker option is spelled (albeit without
the f).

-cary

Re: [google/gcc-4_9] Add gcc driver option -no-pie

[Cary Coutant]

 I'd suggest adding an alias for -no-pie (meaning --no-pie) -- see
 earlier in common.opt where -pie is declared as an alias for pie,
 and similarly for -shared.

 Patch Updated.

OK for google/gcc-4_9 branch. Thanks!

-cary

Re: [PATCH 2/2] PR debug/63240 Add DWARF representation for C++11 defaulted member function.

[Cary Coutant]

 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

[Cary Coutant]

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 hubi...@ucw.cz wrote:
 On Thu, Sep 25, 2014 at 8:37 AM, H.J. Lu hjl.to...@gmail.com wrote:
  On Thu, Sep 25, 2014 at 8:24 AM, Ian Lance Taylor i...@google.com wrote:
  On Wed, Sep 24, 2014 at 6:58 PM, Jan Hubicka hubi...@ucw.cz 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

Re: Avoid privatization of TLS variables

[Cary Coutant]

 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

[google/4_9] Add a new flag bit to .debug_gnu_pubnames entries

[Cary Coutant]

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  ccout...@google.com

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

[Cary Coutant]

 To avoid the unused new discriminator value, I added a map
 found_call_this_line to track whether a call is the first call in a
 source line seen when assigning discriminators. For the first call in
 a source line, its discriminator is 0. For the following calls in the
 same source line, a new discriminator will be used everytime. The new
 patch is attached. Internal perf test and regression test are ok. Is
 it ok for google-4_9?

This seems overly complex to me. I'd think all you need to do is add a
bit to locus_discrim_map (stealing a bit from discriminator ought to
be fine) that indicates whether the next call should increment the
discriminator or not. Something like this:

increase_discriminator_for_locus (location_t locus, bool return_next)
{
  ...
  if (return_next || (*slot)-needs_increment)
{
  (*slot)-discriminator++;
  (*slot)-needs_increment = false;
}
  else
(*slot)-needs_increment = true;
  return (*slot)-discriminator;
}

-cary

Re: [GOOGLE, AUTOFDO] Assign different discriminators to calls with the same lineno

[Cary Coutant]

2014-08-29  Wei Mi  w...@google.com

* 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 w...@google.com wrote:
 On Fri, Aug 29, 2014 at 10:11 AM, Cary Coutant ccout...@google.com 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.

[google/gcc-4_9] Remove skeleton type units that were being produced with -gsplit-dwarf.

[Cary Coutant]

I've backported this patch from trunk r213765.

These sections were originally intended as targets for .gdb_index
entries that needed to point to type units.  Because of the limitations
of the .debug_gnu_pubnames/pubtypes sections with split DWARF, we were
not able to pass along enough information to the gold linker to generate
those index entries properly, and they had to point to the CU instead.
GDB had to deal with that, and was updated a while ago to no longer
depend on the skeleton TU sections at all. This allows us to reduce
object file sizes with split DWARF by about 30%.

Committed to the google/gcc-4_9 branch at r213768.

-cary


gcc/
* dwarf2out.c (get_skeleton_type_unit): Remove.
(output_skeleton_debug_sections): Remove skeleton type units.
(output_comdat_type_unit): Likewise.
(dwarf2out_finish): Likewise.

Re: [GOOGLE, AUTOFDO] Assign different discriminators to calls with the same lineno

[Cary Coutant]

  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

[Cary Coutant]

 +  /* ??? 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 1/6] RTL dwarf2out changes

[Cary Coutant]

 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 2/3] PR other/61321 - demangler crash on casts in template parameters

[Cary Coutant]

 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

[Cary Coutant]

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  ccout...@google.com

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.

[patch] Fix ICEs with -gsplit-dwarf

[Cary Coutant]

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  ccout...@google.com

gcc/
* dwarf2out.c (remove_addr_table_entry): Remove unnecessary hash table
lookup.
(resolve_addr_in_expr): When replacing the rtx in a location list
entry, get a new address table entry.
(dwarf2out_finish): Call index_location_lists even if there are no
addr_index_table entries yet.


diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 8caf940..94e98a1 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -4222,13 +4222,10 @@ add_addr_table_entry (void *addr, enum ate_kind kind)
 static void
 remove_addr_table_entry (addr_table_entry *entry)
 {
-  addr_table_entry *node;
-
   gcc_assert (dwarf_split_debug_info  addr_index_table);
-  node = (addr_table_entry *) htab_find (addr_index_table, entry);
   /* After an index is assigned, the table is frozen.  */
-  gcc_assert (node-refcount  0  node-index == NO_INDEX_ASSIGNED);
-  node-refcount--;
+  gcc_assert (entry-refcount  0  entry-index == NO_INDEX_ASSIGNED);
+  entry-refcount--;
 }
 
 /* Given a location list, remove all addresses it refers to from the
@@ -23102,11 +23099,16 @@ resolve_addr_in_expr (dw_loc_descr_ref loc)
break;
   case DW_OP_GNU_addr_index:
   case DW_OP_GNU_const_index:
-   if ((loc-dw_loc_opc == DW_OP_GNU_addr_index
-|| (loc-dw_loc_opc == DW_OP_GNU_const_index  loc-dtprel))
-resolve_one_addr (loc-dw_loc_oprnd1.val_entry-addr.rtl,
-NULL))
- return false;
+   if (loc-dw_loc_opc == DW_OP_GNU_addr_index
+|| (loc-dw_loc_opc == DW_OP_GNU_const_index  loc-dtprel))
+  {
+rtx rtl = loc-dw_loc_oprnd1.val_entry-addr.rtl;
+if (resolve_one_addr (rtl, NULL))
+  return false;
+remove_addr_table_entry (loc-dw_loc_oprnd1.val_entry);
+loc-dw_loc_oprnd1.val_entry =
+add_addr_table_entry (rtl, ate_kind_rtx);
+  }
break;
   case DW_OP_const4u:
   case DW_OP_const8u:
@@ -24198,18 +24200,23 @@ dwarf2out_finish (const char *filename)
   dwarf_strict ? DW_AT_macro_info : DW_AT_GNU_macros,
   macinfo_section_label);
 
-  if (dwarf_split_debug_info  addr_index_table != NULL)
+  if (dwarf_split_debug_info)
 {
   /* optimize_location_lists calculates the size of the lists,
  so index them first, and assign indices to the entries.
  Although optimize_location_lists will remove entries from
  the table, it only does so for duplicates, and therefore
  only reduces ref_counts to 1.  */
-  unsigned int index = 0;
   index_location_lists (comp_unit_die ());
-  htab_traverse_noresize (addr_index_table,
-  index_addr_table_entry, index);
+
+  if (addr_index_table != NULL)
+{
+  unsigned int index = 0;
+  htab_traverse_noresize (addr_index_table,
+  index_addr_table_entry, index);
+}
 }
+
   if (have_location_lists)
 optimize_location_lists (comp_unit_die ());
 

Re: [patch] Fix ICEs with -gsplit-dwarf

[Cary Coutant]

Any objections to backporting these fixes to the 4.9 branch?

-cary


On Tue, Jul 1, 2014 at 2:37 PM, Cary Coutant ccout...@google.com 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  ccout...@google.com

 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

[Cary Coutant]

 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

[Cary Coutant]

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  ccout...@google.com

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

[Cary Coutant]

 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 pal...@redhat.com

 PR other/61321
 PR other/61233
 * demangle.h (enum demangle_component_type)
 DEMANGLE_COMPONENT_CONVERSION: 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

[patch] PR debug/61013: Change -g so that it will override -g1 but not -g3

[Cary Coutant]

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  ccout...@google.com

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: [patch] PR debug/61013: Change -g so that it will override -g1 but not -g3

[Cary Coutant]

 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

Re: [GOOGLE] Updates highest_location when updating next_discriminator_location

[Cary Coutant]

 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

Re: [GOOGLE] Updates highest_location when updating next_discriminator_location

[Cary Coutant]

 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

[PATCH] Use -ggnu-pubnames with -gsplit-dwarf

[Cary Coutant]

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  ccout...@google.com

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

[google/gcc-4_9] Force the use of -ggnu-pubnames when using -gsplit-dwarf

[Cary Coutant]

I've backported this patch from trunk at r210395.

-cary

gcc/
* opts.c (finish_options): Use -ggnu-pubnames with -gsplit-dwarf.

Re: [GOOGLE] Updates highest_location when updating next_discriminator_location

[Cary Coutant]

 Attached patch passes regression tests and benchmark test. OK for google-4_9?

OK. Thanks again!

-cary

Re: [GOOGLE] backport discriminator support from google-4_8 to google-4_9

[Cary Coutant]

On Mon, May 12, 2014 at 1:11 PM, Dehao Chen de...@google.com 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  de...@google.com

* 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  de...@google.com

* 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  de...@google.com

* 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  de...@google.com

* 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.

Re: [GOOGLE] backport discriminator support from google-4_8 to google-4_9

[Cary Coutant]

 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

[google/gcc-4_8] Add -fskeleton-type-units flag.

[Cary Coutant]

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  ccout...@google.com

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: PR debug/60929: Fix a few ICEs and other problems with -fdebug-types-sections

[Cary Coutant]

What are the rules for backporting to 4.9.1? Should I backport this patch?

-cary


 2014-04-25  Cary Coutant  ccout...@google.com

 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.

[google/gcc-4_9] PR debug/60929: Fix a few ICEs and other problems with -fdebug-types-sections

[Cary Coutant]

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.

PR debug/60929: Fix a few ICEs and other problems with -fdebug-types-sections

[Cary Coutant]

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  ccout...@google.com

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_pointer);
+
replace_child (c, clone, prev

Re: [PATCH] Add DW_AT_const_value as unsigned or int depending on type and value used.

[Cary Coutant]

 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] dwarf2out: Use normal constant values in bound_info if possible.

[Cary Coutant]

 +   /* 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.

[Cary Coutant]

* 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] Add DW_AT_const_value as unsigned or int depending on type and value used.

[Cary Coutant]

 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] PR debug/16063. Add DW_AT_type to DW_TAG_enumeration.

[Cary Coutant]

 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.

[Cary Coutant]

 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

[Cary Coutant]

 ChangeLog:
 2014-03-25  Siva Chandra Reddy  sivachan...@google.com

 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.

[Cary Coutant]

 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

[Cary Coutant]

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  ccout...@google.com

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 template-args 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)

[Cary Coutant]

 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

[Cary Coutant]

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  ccout...@google.com

* 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

[Cary Coutant]

 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)

[Cary Coutant]

 gcc/
 2013-11-24  Tobias Burnus  bur...@net-b.de

   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  bur...@net-b.de

   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 bur...@net-b.de 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):

  11e: 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
  228: 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)
  240: 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)
  258: Abbrev Number: 0
  159: 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)
  160: 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)
  167: Abbrev Number: 5 (DW_TAG_namelist)
 68   DW_AT_name: nml
  26c: Abbrev Number: 6 (DW_TAG_namelist_item)
 6d   DW_AT_namelist_items: 0x28
  271: Abbrev Number: 6 (DW_TAG_namelist_item)
 72   DW_AT_namelist_items: 0x40

 Second file:

   24f: 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)]
  256: Abbrev Number: 4 (DW_TAG_lexical_block)
 57   DW_AT_low_pc  : 0xb
 5f   DW_AT_high_pc : 0xb0
  267: Abbrev Number: 0
  168: Abbrev Number: 5 (DW_TAG_module)
 69   DW_AT_name: mm
 6c   DW_AT_declaration : 1
 6c   DW_AT_sibling : 0x76
  270: Abbrev Number: 6 (DW_TAG_namelist)
 71   DW_AT_name: nml
 75   DW_AT_declaration : 1
  275: 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 enough to see whether that's also the case for the generated
 dwarf.)

 I assume that the compiler can see from the DWARF of the second file that
 nml comes from module mm and doesn't search the value elsewhere. (It is
 possible to have multiple 

Re: [PATCH] Use DW_LANG_D for D

[Cary Coutant]

 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

[Cary Coutant]

 gcc/ChangeLog

 2013-12-02 Sterling Augustine  saugust...@google.com

 * dwarf2out.c (output_pubnames): Use comp_unit_die ()-die_offset
 when there
 isn't a skeleton die.

This is OK, but your patch also has a local change to contrib/mklog.
Please be careful not to commit that.

Thanks!

-cary

Re: [patch] PR 59195: C++ demangler handles conversion operator incorrectly

[Cary Coutant]

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  ccout...@google.com

 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 template-args 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 ccout...@google.com
Date:   Wed Nov 13 09:28:58 2013 -0800

Fix demangler to handle conversion operators correctly.

2013-11-19  Cary Coutant  ccout...@google.com

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 template-args 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 template-template-param template-args.  The
-template-template-param part is a substitution
+ /* This may be template-template-param template-args.
+If this is the type for a conversion

Re: [RFC] Modify -g1 to produce line tables

[Cary Coutant]

 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

[Cary Coutant]

 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: [RFC] Modify -g1 to produce line tables

[Cary Coutant]

 Here, finally, is that patch again, reworked to generate line tables
 at -g1. I plan to commit this when Stage 1 reopens, but I'd like to
 verify that earlier consensus. I also plan to commit this to the
 google/main branch, and future merges will go more smoothly if what I
 put in google/main matches what eventually goes into trunk.

 Hmm,  Stage 1 has been opened for a while now but I could not find
 this patch has been committed yet.  Is there any plans to include this
 patch?  It would be useful for Sanitizer and other uses.

Sorry, I never saw any feedback, positive or negative, on that, and it
kind of fell off my radar. I think it should still be ready to go in
-- Stage 1 is still open for another day, right? Let me rebase the
patch, kick off a bootstrap and regression tests, and I think I can be
ready to submit it if there are no objections.

-cary

Re: [RFC] Modify -g1 to produce line tables

[Cary Coutant]

 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

[Cary Coutant]

 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 ccout...@google.com
Date:   Wed Nov 20 16:02:11 2013 -0800

Add minimal line tables at -g1.

2013-11-20  Cary Coutant  ccout...@google.com

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 column was added in dwarf4.  Simplify the below
@@ -24073,7 +24079,7 @@ dwarf2out_finish (const char *filename)
}
 }
 
-  if (debug_info_level = DINFO_LEVEL_NORMAL

[patch] PR 59195: C++ demangler handles conversion operator incorrectly

[Cary Coutant]

In PR 59195, I describe a demangler problem with parsing
conversion operators that have template parameters:

  http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59195

For example,

  $ c++filt _ZN1AcvPT_I1CEEv

fails -- it should print A::operator C*C().

  $ c++filt _ZN1AcvT_IiEI1CEEv

prints A::operator intintC() instead of
A::operator CintC().

This patch fixes the parser to resolve the ambiguity in the mangled
name grammar where it can't tell if the T_ is a template-param
or is the start of template-template-param template-args, and
fixes the printer to use the correct context when resolving the
template parameter substitution.

Passes all regression tests, and I've added new test cases.

OK to commit?

-cary


2013-11-19  Cary Coutant  ccout...@google.com

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 template-args for
a conversion operator, backtrack if necessary.
(d_expression_1): Renamed from d_expression.
(d_expression): New wrapper around d_expression_1.
(d_checkpoint): New function.
(d_backtrack): New function.
(d_print_init): Initialize current_template.
(d_print_comp): Set current_template.
(d_print_cast): Put current_template in scope for
printing conversion operator name.
(cplus_demangle_init_info): Initialize is_expression and
is_conversion.
* cp-demangle.h (struct d_info): Add is_expression and
is_conversion fields.
* testsuite/demangle-expected: New test cases.


diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c
index 7be9804..edfd85c 100644
--- a/libiberty/cp-demangle.c
+++ b/libiberty/cp-demangle.c
@@ -287,6 +287,19 @@ struct d_saved_scope
   struct d_print_template *templates;
 };
 
+/* Checkpoint structure to allow backtracking.  This holds copies
+   of the fields of struct d_info that need to be restored
+   if a trial parse needs to be backtracked over.  */
+
+struct d_info_checkpoint
+{
+  const char *n;
+  int next_comp;
+  int next_sub;
+  int did_subs;
+  int expansion;
+};
+
 enum { D_PRINT_BUFFER_LENGTH = 256 };
 struct d_print_info
 {
@@ -318,6 +331,8 @@ struct d_print_info
   struct d_saved_scope *saved_scopes;
   /* Number of saved scopes in the above array.  */
   int num_saved_scopes;
+  /* The nearest enclosing template, if any.  */
+  const struct demangle_component *current_template;
 };
 
 #ifdef CP_DEMANGLE_DEBUG
@@ -444,6 +459,10 @@ d_add_substitution (struct d_info *, struct 
demangle_component *);
 
 static struct demangle_component *d_substitution (struct d_info *, int);
 
+static void d_checkpoint (struct d_info *, struct d_info_checkpoint *);
+
+static void d_backtrack (struct d_info *, struct d_info_checkpoint *);
+
 static void d_growable_string_init (struct d_growable_string *, size_t);
 
 static inline void
@@ -1734,8 +1753,15 @@ d_operator_name (struct d_info *di)
   if (c1 == 'v'  IS_DIGIT (c2))
 return d_make_extended_operator (di, c2 - '0', d_source_name (di));
   else if (c1 == 'c'  c2 == 'v')
-return d_make_comp (di, DEMANGLE_COMPONENT_CAST,
-   cplus_demangle_type (di), NULL);
+{
+  struct demangle_component *type;
+
+  if (!di-is_expression)
+di-is_conversion = 1;
+  type = cplus_demangle_type (di);
+  di-is_conversion = 0;
+  return d_make_comp (di, DEMANGLE_COMPONENT_CAST, type, NULL);
+}
   else
 {
   /* LOW is the inclusive lower bound.  */
@@ -2284,13 +2310,61 @@ cplus_demangle_type (struct d_info *di)
   ret = d_template_param (di);
   if (d_peek_char (di) == 'I')
{
- /* This is template-template-param template-args.  The
-template-template-param part is a substitution
+ /* This may be template-template-param template-args.
+If this is the type for a conversion operator, we can
+have a template-template-param here only by following
+a derivation like this:
+
+nested-name
+- template-prefix template-args
+- prefix template-unqualified-name template-args
+- unqualified-name template-unqualified-name template-args
+- source-name template-unqualified-name template-args
+- source-name operator-name template-args
+- source-name cv type template-args
+- source-name cv template-template-param template-args 
template-args
+
+where the template-args is followed by another.
+Otherwise, we must have a derivation like this:
+
+nested-name
+- template-prefix template-args
+- prefix template-unqualified-name template-args
+- unqualified-name template-unqualified-name template-args

Re: [patch] Fix ICEs when DEBUG_MANGLE is enabled

[Cary Coutant]

 2013-11-13  Cary Coutant  ccout...@google.com

 gcc/cp/
 * mangle.c (to_base36): New function.
 (dump_substitution_candidates): Add checks for NULL.
 Print substitution index in base 36.

Ping?

-cary