<html><head><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Jun 21, 2018, at 9:46 AM, Leonard Mosescu <<a href="mailto:mosescu@google.com" class="">mosescu@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline" class="">Leonard, I'm not going to use your patch, as it's a bit un-llvm-y</span><br style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial" class=""><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline" class="">(uses std::ostream and such). However, I wanted to check whether 20</span><br style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial" class=""><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline" class="">bytes will be enough for your use cases (uuids in minidumps)?</span><br class=""></blockquote><div class=""><br class=""></div><div class="">For minidumps we normally use either 16 or 20 byte UUIDs, so I don't see any immediate problems. Are you planning to make 20 a hard limit or have the 20 bytes "inlined" and dynamically allocate if larger?</div></div></div></blockquote><div><br class=""></div>We could use a llvm::SmallVector<uint8_t, 20> to have up to 20 bytes before going larger and allocating on the heap.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Thu, Jun 21, 2018 at 8:18 AM, Pavel Labath <span dir="ltr" class=""><<a href="mailto:labath@google.com" target="_blank" class="">labath@google.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">That sounds like a plan. I have started cleaning up the class a bit<br class="">
(removing manual uuid string formatting in various places and such),<br class="">
and then I'll send a patch which implements that.<br class="">
<br class="">
Leonard, I'm not going to use your patch, as it's a bit un-llvm-y<br class="">
(uses std::ostream and such). However, I wanted to check whether 20<br class="">
bytes will be enough for your use cases (uuids in minidumps)?<br class="">
<div class="HOEnZb"><div class="h5">On Thu, 21 Jun 2018 at 16:03, Greg Clayton <<a href="mailto:clayborg@gmail.com" class="">clayborg@gmail.com</a>> wrote:<br class="">
><br class="">
> I am fine if we go with any number of bytes. We should have the lldb_private::UUID class have an array of bytes that is in the class that is to to 20 bytes. We can increase it later if needed. I would rather not have a dynamically allocated buffer.<br class="">
><br class="">
> That being said a few points:<br class="">
> - Length can be set to zero to indicate invalid UUID. Better that than filling in all zeroes and having to check for that IMHO. I know there were some problems with the last patch around this.<br class="">
> - Don't set length to a valid value and have UUID contain zeros unless that is a true UUID that was calculated. LLDB does a lot of things by matching UUID values so we can't have multiple modules claiming to have a UUID that is filled with zeroes, otherwise many matches will occur that we don't want<br class="">
> - 32 bit GNU debug info CRCs from ELF notes could be filled in as 4 byte UUIDs<br class="">
> - Comparing two UUIDs can start with the length field first the if they match proceed to compare the bytes (which is hopefully what is already happening)<br class="">
><br class="">
><br class="">
> On Jun 20, 2018, at 11:01 AM, Leonard Mosescu via lldb-dev <<a href="mailto:lldb-dev@lists.llvm.org" class="">lldb-dev@lists.llvm.org</a>> wrote:<br class="">
><br class="">
> Here's a snapshot of the old changes I had: <a href="https://reviews.llvm.org/D48381" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/<wbr class="">D48381</a><br class="">
> (hopefully it helps a bit but caveat emptor: this is a quick merge from an old patch, so it's for illustrative purposes only)<br class="">
><br class="">
><br class="">
> On Wed, Jun 20, 2018 at 10:26 AM, Pavel Labath <<a href="mailto:labath@google.com" class="">labath@google.com</a>> wrote:<br class="">
>><br class="">
>> From the looks of it, the patch stalled on the part whether we can<br class="">
>> consider all-zero UUIDs as valid or not. I've dug around the code a<br class="">
>> bit now, and I've found this comment in ObjectFileMachO.cpp.<br class="">
>><br class="">
>> // "main bin spec" (main binary specification) data payload is<br class="">
>> // formatted:<br class="">
>> // uint32_t version [currently 1]<br class="">
>> // uint32_t type [0 == unspecified, 1 ==<br class="">
>> kernel, 2 == user process]<br class="">
>> // uint64_t address [ UINT64_MAX if address not specified ]<br class="">
>> // uuid_t uuid [ all zero's if uuid not specified ]<br class="">
>> // uint32_t log2_pagesize [ process page size in log<br class="">
>> base 2, e.g. 4k pages are 12. 0 for unspecified ]<br class="">
>><br class="">
>><br class="">
>> So it looks like there are situations where we consider all-zero UUIDs<br class="">
>> as invalid.<br class="">
>><br class="">
>> I guess that means we either have to keep IsValid() definition as-is,<br class="">
>> or make ObjectFileMachO check the all-zero case itself. (Some middle<br class="">
>> ground may be where we have two SetFromStringRef functions, one which<br class="">
>> treats all-zero case specially (sets m_num_uuid_bytes to 0), and one<br class="">
>> which doesn't). Then clients can pick which semantics they want.<br class="">
>><br class="">
>><br class="">
>> > 1. A variable-length UUID likely incurs an extra heap allocation.<br class="">
>> Not really. If you're happy with the current <=20 limit, then you can<br class="">
>> just use the existing data structure. Otherwise, you could use a<br class="">
>> SmallVector<uint8_t, 20>.<br class="">
>><br class="">
>> > 2. Formatting arbitrary length UUIDs as string is a bit inconvenient as you noted as well.<br class="">
>> For the string representation, I would say we should just use the<br class="">
>> existing layout of dashes (after 4, 6, 8, 10 and 16 bytes) and just<br class="">
>> cut it short when we have less bytes. The implementation of that<br class="">
>> should be about a dozen lines of code.<br class="">
>><br class="">
>> The fact that these new UUIDs would not be real UUIDs could be solved<br class="">
>> by renaming this class to something else, if anyone can think of a<br class="">
>> good name for it (I can't). Then the "real" UUIDs will be just a<br class="">
>> special case of the new object.<br class="">
>><br class="">
>> pl<br class="">
><br class="">
><br class="">
> ______________________________<wbr class="">_________________<br class="">
> lldb-dev mailing list<br class="">
> <a href="mailto:lldb-dev@lists.llvm.org" class="">lldb-dev@lists.llvm.org</a><br class="">
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev" rel="noreferrer" target="_blank" class="">http://lists.llvm.org/cgi-bin/<wbr class="">mailman/listinfo/lldb-dev</a><br class="">
><br class="">
><br class="">
</div></div></blockquote></div><br class=""></div>
</div></blockquote></div><br class=""></body></html>