<div dir="ltr">I didn't know about llvm::SmallVector, but it seems a good match indeed, thanks Greg.</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 21, 2018 at 9:49 AM, Greg Clayton <span dir="ltr"><<a href="mailto:clayborg@gmail.com" target="_blank">clayborg@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word;line-break:after-white-space"><br><div><span class=""><br><blockquote type="cite"><div>On Jun 21, 2018, at 9:46 AM, Leonard Mosescu <<a href="mailto:mosescu@google.com" target="_blank">mosescu@google.com</a>> wrote:</div><br class="m_8998074364368663628Apple-interchange-newline"><div><div dir="ltr"><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">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"><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">(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"><span style="font-size:12.8px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">bytes will be enough for your use cases (uuids in minidumps)?</span><br></blockquote><div><br></div><div>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></div></span>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><div class="h5"><div><br><blockquote type="cite"><div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 21, 2018 at 8:18 AM, Pavel Labath <span dir="ltr"><<a href="mailto:labath@google.com" target="_blank">labath@google.com</a>></span> wrote:<br><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>
(removing manual uuid string formatting in various places and such),<br>
and then I'll send a patch which implements that.<br>
<br>
Leonard, I'm not going to use your patch, as it's a bit un-llvm-y<br>
(uses std::ostream and such). However, I wanted to check whether 20<br>
bytes will be enough for your use cases (uuids in minidumps)?<br>
<div class="m_8998074364368663628HOEnZb"><div class="m_8998074364368663628h5">On Thu, 21 Jun 2018 at 16:03, Greg Clayton <<a href="mailto:clayborg@gmail.com" target="_blank">clayborg@gmail.com</a>> wrote:<br>
><br>
> 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>
><br>
> That being said a few points:<br>
> - 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>
> - 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>
> - 32 bit GNU debug info CRCs from ELF notes could be filled in as 4 byte UUIDs<br>
> - 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>
><br>
><br>
> On Jun 20, 2018, at 11:01 AM, Leonard Mosescu via lldb-dev <<a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a>> wrote:<br>
><br>
> Here's a snapshot of the old changes I had: <a href="https://reviews.llvm.org/D48381" rel="noreferrer" target="_blank">https://reviews.llvm.org/D4838<wbr>1</a><br>
> (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>
><br>
><br>
> On Wed, Jun 20, 2018 at 10:26 AM, Pavel Labath <<a href="mailto:labath@google.com" target="_blank">labath@google.com</a>> wrote:<br>
>><br>
>> From the looks of it, the patch stalled on the part whether we can<br>
>> consider all-zero UUIDs as valid or not. I've dug around the code a<br>
>> bit now, and I've found this comment in ObjectFileMachO.cpp.<br>
>><br>
>> // "main bin spec" (main binary specification) data payload is<br>
>> // formatted:<br>
>> // uint32_t version [currently 1]<br>
>> // uint32_t type [0 == unspecified, 1 ==<br>
>> kernel, 2 == user process]<br>
>> // uint64_t address [ UINT64_MAX if address not specified ]<br>
>> // uuid_t uuid [ all zero's if uuid not specified ]<br>
>> // uint32_t log2_pagesize [ process page size in log<br>
>> base 2, e.g. 4k pages are 12. 0 for unspecified ]<br>
>><br>
>><br>
>> So it looks like there are situations where we consider all-zero UUIDs<br>
>> as invalid.<br>
>><br>
>> I guess that means we either have to keep IsValid() definition as-is,<br>
>> or make ObjectFileMachO check the all-zero case itself. (Some middle<br>
>> ground may be where we have two SetFromStringRef functions, one which<br>
>> treats all-zero case specially (sets m_num_uuid_bytes to 0), and one<br>
>> which doesn't). Then clients can pick which semantics they want.<br>
>><br>
>><br>
>> > 1. A variable-length UUID likely incurs an extra heap allocation.<br>
>> Not really. If you're happy with the current <=20 limit, then you can<br>
>> just use the existing data structure. Otherwise, you could use a<br>
>> SmallVector<uint8_t, 20>.<br>
>><br>
>> > 2. Formatting arbitrary length UUIDs as string is a bit inconvenient as you noted as well.<br>
>> For the string representation, I would say we should just use the<br>
>> existing layout of dashes (after 4, 6, 8, 10 and 16 bytes) and just<br>
>> cut it short when we have less bytes. The implementation of that<br>
>> should be about a dozen lines of code.<br>
>><br>
>> The fact that these new UUIDs would not be real UUIDs could be solved<br>
>> by renaming this class to something else, if anyone can think of a<br>
>> good name for it (I can't). Then the "real" UUIDs will be just a<br>
>> special case of the new object.<br>
>><br>
>> pl<br>
><br>
><br>
> ______________________________<wbr>_________________<br>
> lldb-dev mailing list<br>
> <a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/lldb-dev</a><br>
><br>
><br>
</div></div></blockquote></div><br></div>
</div></blockquote></div><br></div></div></div></blockquote></div><br></div>