On Thu, Jul 24, 2014 at 7:21 PM, Martin Liška <mli...@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 <dmalc...@redhat.com>
>> Revert my last two changes, r201865 and r201864:
>> Revert r201865:
>> 2013-08-20 David Malcolm <dmalc...@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 <dmalc...@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
>>
>
