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

Wed Feb 25 21:14:31 PST 2015

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.

> 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.

- 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/e78409e2/attachment.html>