<div dir="ltr"><div>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.</div><div><br></div>So lets keep it as is and reconsider locking later.<div><br></div><div>And looks like you don't want attach PR? Maybe it's too late.<br><div><br></div><div><br></div></div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, Sep 9, 2016 at 2:05 PM Filipe Cabecinhas <<a href="mailto:filcab%2Bllvm.phabricator@gmail.com">filcab+llvm.phabricator@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">But then you end up needlessly locking the registry. We only need to<br class="gmail_msg">
lock it when querying the stacks.<br class="gmail_msg">
<br class="gmail_msg">
Since you can call it from user code, I don't think we should be doing<br class="gmail_msg">
that, as we'd be making the critical section much bigger than it<br class="gmail_msg">
should be.<br class="gmail_msg">
Since the other patches depend on this one, you don't see it being<br class="gmail_msg">
used yet, but I've already posted several patches that need it:<br class="gmail_msg">
D24389, D24390, D24391, D24392, D24393. (Those patches aren't yet<br class="gmail_msg">
based on this newer version of this patch. I will updated them soon).<br class="gmail_msg">
<br class="gmail_msg">
Thank you,<br class="gmail_msg">
 Filipe<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
On Fri, Sep 9, 2016 at 9:36 PM, Vitaly Buka <<a href="mailto:vitalybuka@google.com" class="gmail_msg" target="_blank">vitalybuka@google.com</a>> wrote:<br class="gmail_msg">
> I see. This is not on the patch again.<br class="gmail_msg">
> This arg is not obvious, called needs to make decision anyway.<br class="gmail_msg">
><br class="gmail_msg">
> Maybe just remove the arg, and make caller to lock the registry:<br class="gmail_msg">
><br class="gmail_msg">
> AddressDescription(uptr addr, uptr length) {<br class="gmail_msg">
>   asanThreadRegistry().CheckLocked();  // as first line, to fail soon.<br class="gmail_msg">
>   ...<br class="gmail_msg">
> }<br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
> On Fri, Sep 9, 2016 at 11:43 AM Filipe Cabecinhas<br class="gmail_msg">
> <<a href="mailto:filcab%2Bllvm.phabricator@gmail.com" class="gmail_msg" target="_blank">filcab+llvm.phabricator@gmail.com</a>> wrote:<br class="gmail_msg">
>><br class="gmail_msg">
>> __asan_describe_address always needs the lock<br class="gmail_msg">
>> But the Error* descriptions have the lock already and you can't double<br class="gmail_msg">
>> lock.<br class="gmail_msg">
>><br class="gmail_msg">
>>  Filipe<br class="gmail_msg">
>><br class="gmail_msg">
>><br class="gmail_msg">
>> On Friday, 9 September 2016, Vitaly Buka <<a href="mailto:vitalybuka@google.com" class="gmail_msg" target="_blank">vitalybuka@google.com</a>> wrote:<br class="gmail_msg">
>>><br class="gmail_msg">
>>> vitalybuka added inline comments.<br class="gmail_msg">
>>><br class="gmail_msg">
>>> ================<br class="gmail_msg">
>>> Comment at: lib/asan/asan_descriptions.h:197<br class="gmail_msg">
>>> @@ +196,3 @@<br class="gmail_msg">
>>> +  AddressDescription(uptr addr, uptr length,<br class="gmail_msg">
>>> +                     bool shouldLockThreadRegistry = true) {<br class="gmail_msg">
>>> +    if (GetShadowAddressInformation(addr, &data.shadow)) {<br class="gmail_msg">
>>> ----------------<br class="gmail_msg">
>>> And I'd prefer for now remove shouldLockThreadRegistry argument and do<br class="gmail_msg">
>>> ThreadRegistryLock inside of GetStackAddressInformation uconditionally<br class="gmail_msg">
>>><br class="gmail_msg">
>>><br class="gmail_msg">
>>> <a href="https://reviews.llvm.org/D24131" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D24131</a><br class="gmail_msg">
>>><br class="gmail_msg">
>>><br class="gmail_msg">
>>><br class="gmail_msg">
><br class="gmail_msg">
</blockquote></div>