On Wed, Mar 20, 2019 at 7:36 PM Otto, Thomas <thomas.o...@pdv-fs.de> wrote:
>
> > > > > "ggc_collect() discarding/reusing remap_debug_filename() output,
> > > > > thus producing invalid objects"
> > > >
> > > > Hmm, but AFAICS it can end up on the heap if plain get_src_pwd ()
> > > > result survives.  I vaguely remember GC being happy with heap
> > > > strings (due to identifiers?), but not sure.  Otherwise the patch looks
> > > > obvious enough.
> > >
> > > Good point, remap_filename can return the original filename pointer
> > > (which can point to env["PWD"] or to an allocated string) or a ggc string.
> > >
> > > Since not freeing the string pointed to by a static variable is ok (as
> > > getpwd does it), what about checking if remap_filename just returns
> > > the input filename ptr again, and if not copy the ggc string into an 
> > > allocated
> > > buffer.
> >
> > I think we always want GC allocated storage here since the whole dwarf2out
> > data structures live in GC memory.  That means unconditionally copying there
> > as is done inside the DWARF2_DIR_SHOULD_END_WITH_SEPARATOR path.
>
> Explicitly requesting gc storage (A or B below) doesn't seem to work. The 
> mapped
> wd string is not modified this time but is placed in a very different part of 
> the
> assembly, in .section .debug_str.dwo, and after assembling it does not even 
> make it
> into object, not even in a corrupted form as previously.

With the cache variable moved to file scope and marked GTY(())?  That
sounds odd.

The point is that the garbage collector shouldn't end up at heap
allocated storage
when walking GC allocated data and I see the DWARF attribute eventually using
the result of this function has the char * pointer GTY(()) marked.

> Placing the static cached_wd on function or file level makes no difference, 
> only the
> GTY(()) marker fixes it again. Or of course allocating a buffer myself (C).
>
> And while DWARF2_DIR_SHOULD_END_WITH_SEPARATOR is only true on VMS, if I
> enter this if block and do not specify -fdebug-prefix-map then the final 
> object does
> not even contain the working directory, probably because it is stored in
> the ggc_vec_alloc data. Tested with 8.3 built with 4.9 and now also 8.3 
> itself. Maybe
> gc and static storage do not play well together.
>
> That leaves classic allocation of memory and pointing the cached_wd there. It
> really shouldn't matter since it is just a cache of an otherwise side effect 
> free
> function return value. It would outlive all of its callers anyhow, gc or not.
>
>
>   static const char *cached_wd = NULL;
>   // [...]
>   cached_wd = remap_debug_filename (wd);
>   if (cached_wd != wd)
>     {
>       size_t cached_wd_len = strlen (cached_wd);
> A:        char *buf = ggc_vec_alloc<char> (cached_wd_len);
> B:        char *buf = (char*)ggc_alloc_atomic (cached_wd_len);
> C:        char *buf = (char*)xmalloc (cached_wd_len);
>       memcpy (buf, cached_wd, cached_wd_len);
>       cached_wd = buf;
>     }
>

Reply via email to