Re: Symtab node table introduction and GGC issues

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

On Fri, Jul 25, 2014 at 12:24 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Jul 24, 2014 at 7:21 PM, Martin LiÅka <mliska@suse.cz> wrote:
>>
>> On 07/24/2014 05:53 PM, David Malcolm wrote:
>>>
>>> On Thu, 2014-07-24 at 16:23 +0100, Martin LiÅka wrote:
>>>>
>>>> Hello,
>>>>      I would like to encapsulate all symtab_nodes to a class called
>>>> 'symtab'.
>>>
>>> Thanks!
>>>
>>>>   To respect gcc:context, I would like to add the class to global
>>>> context (gcc::context) and I have troubles with GTY and GGC.
>>>>
>>>> design:
>>>>
>>>> class GTY(()) symtab
>>>> {
>>>> public:
>>>>     symtab_node* GTY(()) nodes;
>>>> };
>>>>
>>>> class GTY(()) context
>>>> {
>>>> public:
>>>>     context () {}
>>>>     void init ();
>>>>
>>>>     /* Pass-management.  */
>>>>     pass_manager *get_passes () { gcc_assert (m_passes); return m_passes;
>>>> }
>>>>
>>>>     /* Handling dump files.  */
>>>>     dump_manager *get_dumps () {gcc_assert (m_dumps); return m_dumps; }
>>>>
>>>>     /* Symbol table.  */
>>>>     symtab *get_symtab () { gcc_assert (symbol_table); return
>>>> symbol_table; }
>>>>
>>>>     /* Symbol table.  */
>>>>     symtab * GTY(()) symbol_table;
>>>>
>>>> private:
>>>>     /* Pass-management.  */
>>>>     pass_manager * GTY((skip)) m_passes;
>>>>
>>>>     /* Dump files.  */
>>>>     dump_manager * GTY((skip)) m_dumps;
>>>> }; // class context
>>>>
>>>> } // namespace gcc
>>>>
>>>> /* The global singleton context aka "g".
>>>>      (the name is chosen to be easy to type in a debugger).  */
>>>> extern GTY(()) gcc::context *g;
>>>>
>>>>
>>>>
>>>> gcc:context in created by g = ggc_cleared_alloc<gcc::context> (); and
>>>> g->symbol_table too.
>>>> My problem is that during pg_pch_restore:
>>>> #3  0x0000000000850043 in gt_pch_restore (f=0x1ac9100) at
>>>> ../../gcc/ggc-common.c:691
>>>>
>>>> When I add watch to *(&g), gcc::context *g is restored ^ and
>>>> gcc:context::m_passes and gcc:context::m_dumps are set to an inaccessible
>>>> address.
>>>>
>>>> Is my GGC construct correct?
>>>
>>> Back when we introduced the context and the pass_manager (called the
>>> "pipeline" in the initial patch submissions), those two classes were
>>> GTY-marked, but I reverted the change in r201887 (aka
>>> 5d068519b66a37ab391c427d0aac13f66a9b5c4e):
>>>
>>>      Revert my last two changes, r201865 and r201864
>>>           2013-08-20  David Malcolm  <dmalcolm@redhat.com>
>>>               Revert my last two changes, r201865 and r201864:
>>>               Revert r201865:
>>>          2013-08-20  David Malcolm  <dmalcolm@redhat.com>
>>>               Make opt_pass and gcc::pass_manager be GC-managed, so that
>>> pass
>>>          instances can own GC refs.
>>>               * Makefile.in (GTFILES): Add pass_manager.h and tree-pass.h.
>>>          * context.c (gcc::context::gt_ggc_mx): Traverse passes_.
>>>          (gcc::context::gt_pch_nx): Likewise.
>>>          (gcc::context::gt_pch_nx):  Likewise.
>>>          * ggc.h (gt_ggc_mx <T>): New.
>>>          (gt_pch_nx_with_op <T>): New.
>>>          (gt_pch_nx <T>): New.
>>>          * passes.c (opt_pass::gt_ggc_mx): New.
>>>          (opt_pass::gt_pch_nx): New.
>>>          (opt_pass::gt_pch_nx_with_op): New.
>>>          (pass_manager::gt_ggc_mx): New.
>>>          (pass_manager::gt_pch_nx): New.
>>>          (pass_manager::gt_pch_nx_with_op): New.
>>>          (pass_manager::operator new): Use
>>>          ggc_internal_cleared_alloc_stat rather than xcalloc.
>>>          * pass_manager.h (class pass_manager): Add GTY((user)) marking.
>>>          (pass_manager::gt_ggc_mx): New.
>>>          (pass_manager::gt_pch_nx): New.
>>>          (pass_manager::gt_pch_nx_with_op): New.
>>>          * tree-pass.h (class opt_pass): Add GTY((user)) marking.
>>>          (opt_pass::operator new): New.
>>>          (opt_pass::gt_ggc_mx): New.
>>>          (opt_pass::gt_pch_nx): New.
>>>          (opt_pass::gt_pch_nx_with_op): New.
>>>               Revert r201864:
>>>          2013-08-20  David Malcolm  <dmalcolm@redhat.com>
>>>               * Makefile.in (GTFILES): Add context.h.
>>>          * context.c (gcc::context::operator new): New.
>>>          (gcc::context::gt_ggc_mx): New.
>>>          (gcc::context::gt_pch_nx): New.
>>>          (gcc::context::gt_pch_nx): New.
>>>          * context.h (gcc::context): Add GTY((user)) marking.
>>>          (gcc::context::operator new): New.
>>>          (gcc::context::gt_ggc_mx): New.
>>>          (gcc::context::gt_pch_nx): New.
>>>          (gcc::context::gt_pch_nx): New.
>>>          (g): Add GTY marking.
>>>          (gt_ggc_mx (gcc::context *)): New.
>>>          (gt_pch_nx (gcc::context *)): New.
>>>          (gt_pch_nx (gcc::context *ctxt, gt_pointer_operator op,
>>>          void *cookie)): New.
>>>          * gengtype.c (open_base_files) <ifiles>: Add context.h.
>>>
>>> Looking over the list archives, I found this reason for the revert:
>>> https://gcc.gnu.org/ml/gcc/2013-08/msg00214.html
>>> the integration of GTY into context and pass_manager wasn't fully-baked,
>>> and was causing PCH crashes.  Looks like I didn't go back and fully
>>> debug the issue; sorry.
>>>
>>> As for the issue you're seeing... I'm guessing you've put the "skip"
>>> clauses on m_passes and m_dumps as a workaround for the issue from last
>>> year, but they don't seem in themselves to be valid: the context is
>>> GTY-marked, and hence saved to/restored from PCH (albeit buggily,
>>> presumably), but the skip clauses mean that those fields aren't
>>> interacting with PCH, merely copied as raw bytes to/from disk, and so
>>> those pointers are going to be pointing to the address they pointed to
>>> in the process that created the PCH, which will likely be nonsensical in
>>> the process reading the PCH file, which would explain the symptoms
>>> you're seeing.
>>>
>>> For now, maybe keep the symtab ptr as a GTY-marked global, outside of
>>> the context?  We can at least get a big reduction of the global state
>>> that way, and we can revisit GTY-enabling the context as a separate
>>> issue, moving the symtab ptr into the context at that time?
>>
>> Hello,
>>    thank you for you advice. It really looks that I face the same issue as
>> you seen. As you suggested, I will start with a global pointer to symtab and
>> we'll see further integration into context class (where it should reside
>> according to me).
>
> Note that I would suggest an explicit marking function for this singleton
> (thus GTY((user))).  Most pointers of the symtab stuff doesn't need following
> but everything is held together by the single array/map?

Which also would make it possible to move the symtab objects out-of-GC
space (but still follow the ->decl edges).

Richard.

> Richard.
>
>> Thanks,
>> Martin
>>
>>
>>>
>>> Hope this is helpful
>>> Dave
>>>
>>