Symtab node table introduction and GGC issues

[David Malcolm]

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?

Hope this is helpful
Dave