Re: [PATCH] New lang hook

[Richard Biener <richard dot guenther at gmail dot com>]

On Tue, Nov 14, 2017 at 1:31 PM, Nathan Sidwell <nathan@acm.org> wrote:
> This patch addresses c++/82836 & c++/82737.  The root cause was a bad
> assumption I made when moving the mangling alias machinery to its own hash
> table.
>
> I had thought that once we SET_DECL_ASSEMBLER_NAME, it never becomes unset
> (or changed).  That is false.  There are paths in the compiler that set it
> back to zero, and at least one path where we remangled because of a bad
> assumption about the templatedness of a friend.
>
> Previously, we placed mangling aliases in the global namespace mapping an
> identifier (the mangled name) to the decl.  Resetting the assembler name
> didn't break this map -- but may have led to unneeded aliases, I guess.
>
> Moving the alias machinery to its own hash-map allowed me to make namespace
> hashes a simple hash, keyed by DECL_NAME (rather than a hash-map from
> identifier->decl).
>
> Finally converting the alias hash-map to a hash-table keyed by
> DECL_ASSEMBLER_NAME shrunk that table too.  But exposed this problem.
>
> I did contemplate reverting that change, but tried this first, and it seems
> good.
>
> 1) rename the args of COPY_DECL_ASSEMBLER_NAME from DECL1 & DECL2 to the
> more semantic SRC_DECL and DST_DECL.  Same for COPY_DECL_RTL.  These macros
> smell rather memcpy-like, but the args are the wrong way round, so the more
> clues the better. (My comment in 82737 was misled by this.)
>
> 2) change SET_DECL_ASSEMBLER_NAME to a function call, similar to
> DECL_ASSEMBLER_NAME.
>
> 3) have that call forward to a new lang hook, override_decl_assembler_name,
> if it is changing the name. (set_decl_assembler_name already having been
> taken.)
>
> 4) the new lang hook default simply stores the new name via
> DECL_ASSEMBLER_NAME_RAW.
>
> 5) the C++ FE overrides this.  If the current name is in the alias map and
> maps to this decl, we take it out of the map.  Then set the new name.
>
> 6) excitingly, mangle_decl can be called with a non-null
> DECL_ASSEMBLER_NAME, so that function's use of SET_DECL_ASSEMBLER_NAME works
> just fine.
>
> booted on all languages.
>
> ok?

Looks reasonable apart from

+  /* Overwrite the DECL_ASSEMBLER_NAME for a node.  The name is being
+     changed (including to or from NULL_TREE).  */

which suggests the default implementation of set_decl_assembler_name would
call this hook (which it doesn't).  Any particular reason?  Maybe just document
(including to NULL_TREE), thus exclude from NULL_TREE?

Thanks,
Richard.

>
> nathan
> --
> Nathan Sidwell