I didn't know about llvm::SmallVector, but it seems a good match indeed, thanks Greg.
On Thu, Jun 21, 2018 at 9:49 AM, Greg Clayton <clayb...@gmail.com> wrote: > > > On Jun 21, 2018, at 9:46 AM, Leonard Mosescu <mose...@google.com> wrote: > > Leonard, I'm not going to use your patch, as it's a bit un-llvm-y >> (uses std::ostream and such). However, I wanted to check whether 20 >> bytes will be enough for your use cases (uuids in minidumps)? >> > > 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? > > > We could use a llvm::SmallVector<uint8_t, 20> to have up to 20 bytes > before going larger and allocating on the heap. > > > On Thu, Jun 21, 2018 at 8:18 AM, Pavel Labath <lab...@google.com> wrote: > >> That sounds like a plan. I have started cleaning up the class a bit >> (removing manual uuid string formatting in various places and such), >> and then I'll send a patch which implements that. >> >> Leonard, I'm not going to use your patch, as it's a bit un-llvm-y >> (uses std::ostream and such). However, I wanted to check whether 20 >> bytes will be enough for your use cases (uuids in minidumps)? >> On Thu, 21 Jun 2018 at 16:03, Greg Clayton <clayb...@gmail.com> wrote: >> > >> > 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. >> > >> > That being said a few points: >> > - 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. >> > - 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 >> > - 32 bit GNU debug info CRCs from ELF notes could be filled in as 4 >> byte UUIDs >> > - 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) >> > >> > >> > On Jun 20, 2018, at 11:01 AM, Leonard Mosescu via lldb-dev < >> lldb-dev@lists.llvm.org> wrote: >> > >> > Here's a snapshot of the old changes I had: >> https://reviews.llvm.org/D48381 >> > (hopefully it helps a bit but caveat emptor: this is a quick merge from >> an old patch, so it's for illustrative purposes only) >> > >> > >> > On Wed, Jun 20, 2018 at 10:26 AM, Pavel Labath <lab...@google.com> >> wrote: >> >> >> >> From the looks of it, the patch stalled on the part whether we can >> >> consider all-zero UUIDs as valid or not. I've dug around the code a >> >> bit now, and I've found this comment in ObjectFileMachO.cpp. >> >> >> >> // "main bin spec" (main binary specification) data payload >> is >> >> // formatted: >> >> // uint32_t version [currently 1] >> >> // uint32_t type [0 == unspecified, 1 == >> >> kernel, 2 == user process] >> >> // uint64_t address [ UINT64_MAX if address not >> specified ] >> >> // uuid_t uuid [ all zero's if uuid not >> specified ] >> >> // uint32_t log2_pagesize [ process page size in log >> >> base 2, e.g. 4k pages are 12. 0 for unspecified ] >> >> >> >> >> >> So it looks like there are situations where we consider all-zero UUIDs >> >> as invalid. >> >> >> >> I guess that means we either have to keep IsValid() definition as-is, >> >> or make ObjectFileMachO check the all-zero case itself. (Some middle >> >> ground may be where we have two SetFromStringRef functions, one which >> >> treats all-zero case specially (sets m_num_uuid_bytes to 0), and one >> >> which doesn't). Then clients can pick which semantics they want. >> >> >> >> >> >> > 1. A variable-length UUID likely incurs an extra heap allocation. >> >> Not really. If you're happy with the current <=20 limit, then you can >> >> just use the existing data structure. Otherwise, you could use a >> >> SmallVector<uint8_t, 20>. >> >> >> >> > 2. Formatting arbitrary length UUIDs as string is a bit inconvenient >> as you noted as well. >> >> For the string representation, I would say we should just use the >> >> existing layout of dashes (after 4, 6, 8, 10 and 16 bytes) and just >> >> cut it short when we have less bytes. The implementation of that >> >> should be about a dozen lines of code. >> >> >> >> The fact that these new UUIDs would not be real UUIDs could be solved >> >> by renaming this class to something else, if anyone can think of a >> >> good name for it (I can't). Then the "real" UUIDs will be just a >> >> special case of the new object. >> >> >> >> pl >> > >> > >> > _______________________________________________ >> > lldb-dev mailing list >> > lldb-dev@lists.llvm.org >> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev >> > >> > >> > > >
_______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
