[PATCH] [PATCH/RFC] Use a BumpAllocator for DIEs

David Blaikie dblaikie at gmail.com
Wed Feb 25 21:28:41 PST 2015


On Wed, Feb 25, 2015 at 9:20 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2015 Feb 25, at 21:14, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Wed, Feb 25, 2015 at 7:15 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > > On 2015 Feb 19, at 07:39, Frédéric Riss <friss at apple.com> wrote:
> > >
> > > ping ?
> > >
> > >> On Feb 12, 2015, at 11:52 AM, Frédéric Riss <friss at apple.com> wrote:
> > >>
> > >>>
> > >>> On Feb 6, 2015, at 10:28 AM, David Blaikie <dblaikie at gmail.com>
> wrote:
> > >>>
> > >>> Yeah, I'm punting on the language lawyering to Richard, hopefully…
> > >>
> > >> Richard,
> > >>
> > >> no need to look at the patch, but we just need you opinion on the
> following scenario:
> > >>
> > >> What the patch does is allocate all the class DIE objects using a
> BumpPtrAllocator to
> > >> be able to bulk deallocate them. This objects contain a SmallVector
> that in some rare
> > >> cases will resort to heap allocation to store their contents. I added
> bookkeeping to
> > >> keep track of the objects that need their destructors called to free
> the heap allocation.
> > >>
> > >> Basically the code is
> > >> std::vector<DIE *> DIEsToDelete
> > >> …
> > >> for (auto *DIE : DIEsToDelete)
> > >>    DIE->~DIE();
> > >>
> > >> There is a catch though. Some of these objects are of type DIEBlock
> or DIELoc which
> > >> use an identical multiple inheritance scheme:
> > >>
> > >> class DIEBlock : public DIEValue, public DIE {
> > >> …
> > >> };
> > >>
> > >> I’m storing a pointer to these in the above list and thus invoking
> just the DIE
> > >> destructor on them. The objects aren’t used afterwards as the next
> step is to
> > >> free the memory allocated by the BumpPtrAllocator.
> > >>
> > >> David was worried that this might be UB. Any definitive opinion?
> >
> > I'm not Richard, but I don't see anything in the standard to
> > suggest this is UB.  It seems like the relevant section of the
> > standard should be "12.4 Destructors", and explicit destructor
> > calls are described in point 12.
> >
> > > In an explicit destructor call, the destructor name appears as a ̃
> followed by a type-name or decltype- specifier that denotes the
> destructor’s class type. The invocation of a destructor is subject to the
> usual rules for member functions (9.3), that is, if the object is not of
> the destructor’s class type and not of a class derived from the
> destructor’s class type, the program has undefined behavior (except that
> invoking delete on a null pointer has no effect).
> >
> > I read through the rest of the section as well; I don't see any
> > indication that this is invalid.
> >
> > "12.7 Construction and destruction" also looks relevant, but I
> > can't find anything suspicious there either.
> >
> > @David, why do you suspect this is UB?
> >
> > Just seemed sufficiently suspicious that I was willing to punt to
> someone more authoritative, but I'll leave it to you guys if you reckon it
> seems reasonable.
>
> Okay, cool.  After my research today, I'm confident this is fine
> (famous last words, maybe) so I'll LGTM that part.
>
> I haven't looked at the patch itself recently... not sure if that
> was the only blocker?
>

I hadn't really looked at it in any great detail personally. It is some
unfortunate added complexity, but I haven't given thought to
alternatives/improvements.


>
> > Any reason not to trust
> > the UBSan bots to catch problems?
> >
> > I don't really expect UBSan to be complete, though I don't know the
> specific set of checks it does have - so I usually assume it won't catch
> particular bugs unless I'm specifically aware of its support for that bug
> type. Happy to be corrected/enlightened.
>
> Fair point!
>
> >
> > - David
> >
> >
> > >>
> > >>> On Thu, Feb 5, 2015 at 9:25 AM, Frédéric Riss <friss at apple.com>
> wrote:
> > >>> ping?
> > >>>
> > >>>> On Jan 27, 2015, at 6:43 PM, Frédéric Riss <friss at apple.com> wrote:
> > >>>>
> > >>>>>
> > >>>>> On Jan 27, 2015, at 4:11 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> On Tue, Jan 27, 2015 at 2:08 PM, Frederic Riss <friss at apple.com>
> wrote:
> > >>>>> This update removes the limitation to a fixed maximum number of
> > >>>>> attributes per DIE. The patch is very similar to the first one that
> > >>>>> I sent for discussion. It only adds a pointer to a vector of DIEs
> > >>>>> as arguement to DIE::addValue(). If a DIE overflows its inline
> storage
> > >>>>> for attributes, it is added to the vector. The vector is then
> iterated
> > >>>>> just before the BumpPtrAllocator is destroyed to call the
> destructors
> > >>>>> of all these DIEs.
> > >>>>>
> > >>>>> This approach gives the most flexibilty as it doesn't impose the
> > >>>>> memory management upon the user (for example DIEHashTest.cpp
> continues
> > >>>>> to use stack allocated DIEs and it works fine).
> > >>>>>
> > >>>>> I've been playing with this for the last few days, and I have quite
> > >>>>> a few different implementations lying around (for example using
> > >>>>> std::vectors with a custom stateful allocator). They are all more
> > >>>>> complicated and don't perform as well as this patch.
> > >>>>>
> > >>>>> There's one point that I'd like to mention: the patch unifies the
> > >>>>> existing bookkeeping of the DIEBlock and DIELoc objects and treats
> > >>>>> them the same as standard DIEs. This means that we will call ~DIE
> > >>>>> on DIEBlock and DIELoc objects which will result in a 'partial'
> > >>>>> destruction (DIEBlock inherits both from DIEValue and from DIE).
> > >>>>> I think this is fine. It should do exactly what we want and just
> > >>>>> call the destructors of the SmallVectors in the DIE part of the
> > >>>>> object, but I wanted to mention it in case someone thinks its not
> > >>>>> legal to do that.
> > >>>>>
> > >>>>> Pretty sure that's not valid C++ (not sure quite which cracks the
> bump allocator objects fall in general in terms of not running dtors,
> reusing memory, etc - and whether this would be worse than that or not,
> though).
> > >>>>
> > >>>> What exactly do you think the issue would be from a language
> standpoint? Calling a destructor on a ‘partial’ object? I would expect that
> it is ok to do so. AFAIK, explicit destructor calls follow the same rules
> as other function calls. I have a pointer to a DIE object, thus I can call
> the destructor on it. The object stops to ‘exist’ at that point, and thus
> its enclosing object is in an undefined state, but this doesn’t really
> matter as it won’t be touched anymore.
> > >>>>
> > >>>> I’m very bad at language lawyering though, thus I’d really
> appreciate a authoritative answer :-)
> > >>>>
> > >>>> Fred
> > >>>>
> > >>>>>
> > >>>>>
> > >>>>> http://reviews.llvm.org/D7072
> > >>>>>
> > >>>>> Files:
> > >>>>>   include/llvm/CodeGen/DIE.h
> > >>>>>   lib/CodeGen/AsmPrinter/DIE.cpp
> > >>>>>   lib/CodeGen/AsmPrinter/DIEHash.cpp
> > >>>>>   lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
> > >>>>>   lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
> > >>>>>   lib/CodeGen/AsmPrinter/DwarfDebug.cpp
> > >>>>>   lib/CodeGen/AsmPrinter/DwarfFile.cpp
> > >>>>>   lib/CodeGen/AsmPrinter/DwarfUnit.cpp
> > >>>>>   lib/CodeGen/AsmPrinter/DwarfUnit.h
> > >>>>>   unittests/CodeGen/DIEHashTest.cpp
> > >>>>>
> > >>>>> EMAIL PREFERENCES
> > >>>>>   http://reviews.llvm.org/settings/panel/emailpreferences/
> > >>>>
> > >>>> _______________________________________________
> > >>>> llvm-commits mailing list
> > >>>> llvm-commits at cs.uiuc.edu
> > >>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> > >>>
> > >>>
> > >>
> > >> _______________________________________________
> > >> llvm-commits mailing list
> > >> llvm-commits at cs.uiuc.edu
> > >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> > >
> > > _______________________________________________
> > > llvm-commits mailing list
> > > llvm-commits at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150225/c9b6930f/attachment.html>


More information about the llvm-commits mailing list