Re: Symtab node table introduction and GGC issues

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

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?

Richard.

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