[PATCH] D24131: Add NewAddressDescription, which can describe any type of address.

[Vitaly Buka via llvm-commits]

Not big difference, but in general i'd prefer this committed with
unconditional locking, and with no argument. And argument is being added in
the patch where you use if from coded where it set to false.

So lets keep it as is and reconsider locking later.

And looks like you don't want attach PR? Maybe it's too late.



On Fri, Sep 9, 2016 at 2:05 PM Filipe Cabecinhas <
filcab+llvm.phabricator at gmail.com> wrote:

> But then you end up needlessly locking the registry. We only need to
> lock it when querying the stacks.
>
> Since you can call it from user code, I don't think we should be doing
> that, as we'd be making the critical section much bigger than it
> should be.
> Since the other patches depend on this one, you don't see it being
> used yet, but I've already posted several patches that need it:
> D24389, D24390, D24391, D24392, D24393. (Those patches aren't yet
> based on this newer version of this patch. I will updated them soon).
>
> Thank you,
>  Filipe
>
>
> On Fri, Sep 9, 2016 at 9:36 PM, Vitaly Buka <vitalybuka at google.com> wrote:
> > I see. This is not on the patch again.
> > This arg is not obvious, called needs to make decision anyway.
> >
> > Maybe just remove the arg, and make caller to lock the registry:
> >
> > AddressDescription(uptr addr, uptr length) {
> >   asanThreadRegistry().CheckLocked();  // as first line, to fail soon.
> >   ...
> > }
> >
> >
> > On Fri, Sep 9, 2016 at 11:43 AM Filipe Cabecinhas
> > <filcab+llvm.phabricator at gmail.com> wrote:
> >>
> >> __asan_describe_address always needs the lock
> >> But the Error* descriptions have the lock already and you can't double
> >> lock.
> >>
> >>  Filipe
> >>
> >>
> >> On Friday, 9 September 2016, Vitaly Buka <vitalybuka at google.com> wrote:
> >>>
> >>> vitalybuka added inline comments.
> >>>
> >>> ================
> >>> Comment at: lib/asan/asan_descriptions.h:197
> >>> @@ +196,3 @@
> >>> +  AddressDescription(uptr addr, uptr length,
> >>> +                     bool shouldLockThreadRegistry = true) {
> >>> +    if (GetShadowAddressInformation(addr, &data.shadow)) {
> >>> ----------------
> >>> And I'd prefer for now remove shouldLockThreadRegistry argument and do
> >>> ThreadRegistryLock inside of GetStackAddressInformation uconditionally
> >>>
> >>>
> >>> https://reviews.llvm.org/D24131
> >>>
> >>>
> >>>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160909/449201be/attachment.html>