Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Jim Ingham via lldb-commits]

How would the ObjectFile API's that take or return UUID's work in this case?

Jim


> On Nov 28, 2017, at 11:44 AM, Zachary Turner via lldb-commits 
>  wrote:
> 
> Also worth pointing out that when you write things this way, this UUID 
> class can be part of a larger structure that matches the record layout of a 
> header or section in a binary file, and then you can just memcpy over the 
> class and your'e good to go.  For example you could have
> 
> ```
> struct MachOHeader {
>...
>...
>UUID<16> uuid;
>...
> };
> 
> MachOHeader H;
> ::memcpy(, File, sizeof(H));
> File += sizeof(H);
> ```
> 
> and you could do the same for some ELF structure.
> 
> ```
> struct ElfHeader {
>...
>...
>   union {
> UUID<20> BuildId;
> UUID<4> DebugCrc;
>   }
>...
> }; 
> 
> ElfHeader H;
> ::memcpy(, File, sizeof(H));
> File += sizeof(H);
> ```
> 
> And everything just works.
> 
> On Tue, Nov 28, 2017 at 11:36 AM Zachary Turner  wrote:
> Eh, that actually just makes me think the compiler *can* check it.  For 
> example, right now you can have mach-o files with 20 byte UUIDs.  But just in 
> the code, not in practice.  You could have a bug in your code that 
> accidentally wrote the wrong number of bytes from a dynamic buffer.  
> 
> You could enforce this at the compiler level by saying:
> 
> class ObjectFileMachO {
>   UUID<16> uuid;
> };
> 
> Not only is this more correct, but it is less error prone and is also nice 
> documentation to the reader who may be just learning about MachO that this 
> UUID is always 16 bytes.
> 
> For the case of ELF, it sounds like you either have a 20 byte UUID or a 4 
> byte UUID, but never both, and never any other size.  That makes me think of:
> 
> class ObjectFileELF {
>   union {
> UUID<20> BuildId;
> UUID<4> DebugCrc;
>   }
> };
> 
> And now the person reading this code can immediately tell that there will 
> either be one or the other, and depending on which one it is, he/she already 
> knows something about it, like how many bytes it is and what it represents.
> 
> To me this is much more clear than:
> 
> class ObjectFileELF {
>   // This might not actually be a build id, and it could be a variable size, 
> and you also have to be careful
>   // not to put some strange number of bytes in it that we don't recognize, 
> but it's up to the user
>   // to know under what circumstances it should be a certain number of bytes, 
> and you should also always
>   // be careful ensure that there's no buffer overruns since you'll be 
> working with dynamically sized buffers
>   // and the compiler can't warn you when you're doing something wrong.
>   UUID BuildIdOrDebugCrc;
> };
> 
> On Tue, Nov 28, 2017 at 11:06 AM Greg Clayton  wrote:
> 
>> On Nov 28, 2017, at 10:24 AM, Zachary Turner  wrote:
>> 
>> 
>> 
>> On Tue, Nov 28, 2017 at 10:18 AM Greg Clayton  wrote:
>>> On Nov 27, 2017, at 10:11 PM, Zachary Turner  wrote:
>>> 
>>> As an aside, I don't really like this class.  For example, You can 
>>> currently assign a UUID[16] to a UUID[20].  That doesn't make a lot of 
>>> sense to me.
>> 
>> What about an invalid UUID[0] being assigned with a valid UUID[16] or 
>> UUID[20]? Why doesn't this make sense? I don't follow.
>> 
>> Nothing is invalid, I just think it's better and expresses the intent more 
>> clearly if you can only assign between UUIDs of the same size.  For example, 
>> If the UUID class were templated on size, then there would not even be such 
>> thing as a UUID[0] or a "universally invalid UUID".  There would be an 
>> "invalid 16-byte UUID" and an "invalid 20-byte UUID", and those would be 
>> different things.
>>  
>> 
>>> 
>>> As a future cleanup, I think this class should probably be a template such 
>>> as UUID, and then internally it can store a std::array.  And 
>>> we can static_assert that N is of a known size if we desire.
>> 
>> UUID values are objects contained as members inside of other objects. They 
>> all default to start with no preconceived notion of what the UUID should be. 
>> IMHO the UUID class is just fine and needs to be able to represent any UUID, 
>> from empty uninitialized ones, and be able to be assigned and changed at 
>> will.
>> 
>> 
>> Is there ever a use case for changing the number of bytes in a UUID?  If 
>> you're working with 16-byte UUIDs, does it ever actually happen that now you 
>> have a 20-byte UUID?  Can you imagine a use case currently where an N-byte 
>> UUID is being compared against an M-byte UUID in a real-world scenario?  If 
>> the answer is no, then it may as well be enforced by the compiler. 
> 
> The ObjectFile class has a "UUID m_uuid;" member that any object file can 
> fill in. Right now mach-o files have 16 byte UUIDs. ELF files can have 20 
> bytes UUIDs (build ID) or 4 byte UUIDs (debug info CRC if no build ID is 
> around, and these are 

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Zachary Turner via lldb-commits]

Also worth pointing out that when you write things this way, this UUID
class can be part of a larger structure that matches the record layout of a
header or section in a binary file, and then you can just memcpy over the
class and your'e good to go.  For example you could have

```
struct MachOHeader {
   ...
   ...
   UUID<16> uuid;
   ...
};

MachOHeader H;
::memcpy(, File, sizeof(H));
File += sizeof(H);
```

and you could do the same for some ELF structure.

```
struct ElfHeader {
   ...
   ...
  union {
UUID<20> BuildId;
UUID<4> DebugCrc;
  }
   ...
};

ElfHeader H;
::memcpy(, File, sizeof(H));
File += sizeof(H);
```

And everything just works.

On Tue, Nov 28, 2017 at 11:36 AM Zachary Turner  wrote:

> Eh, that actually just makes me think the compiler *can* check it.  For
> example, right now you can have mach-o files with 20 byte UUIDs.  But just
> in the code, not in practice.  You could have a bug in your code that
> accidentally wrote the wrong number of bytes from a dynamic buffer.
>
> You could enforce this at the compiler level by saying:
>
> class ObjectFileMachO {
>   UUID<16> uuid;
> };
>
> Not only is this more correct, but it is less error prone and is also nice
> documentation to the reader who may be just learning about MachO that this
> UUID is always 16 bytes.
>
> For the case of ELF, it sounds like you either have a 20 byte UUID or a 4
> byte UUID, but never both, and never any other size.  That makes me think
> of:
>
> class ObjectFileELF {
>   union {
> UUID<20> BuildId;
> UUID<4> DebugCrc;
>   }
> };
>
> And now the person reading this code can immediately tell that there will
> either be one or the other, and depending on which one it is, he/she
> already knows something about it, like how many bytes it is and what it
> represents.
>
> To me this is much more clear than:
>
> class ObjectFileELF {
>   // This might not actually be a build id, and it could be a variable
> size, and you also have to be careful
>   // not to put some strange number of bytes in it that we don't
> recognize, but it's up to the user
>   // to know under what circumstances it should be a certain number of
> bytes, and you should also always
>   // be careful ensure that there's no buffer overruns since you'll be
> working with dynamically sized buffers
>   // and the compiler can't warn you when you're doing something wrong.
>   UUID BuildIdOrDebugCrc;
> };
>
> On Tue, Nov 28, 2017 at 11:06 AM Greg Clayton  wrote:
>
>>
>> On Nov 28, 2017, at 10:24 AM, Zachary Turner  wrote:
>>
>>
>>
>> On Tue, Nov 28, 2017 at 10:18 AM Greg Clayton  wrote:
>>
>>> On Nov 27, 2017, at 10:11 PM, Zachary Turner  wrote:
>>>
>>> As an aside, I don't really like this class.  For example, You can
>>> currently assign a UUID[16] to a UUID[20].  That doesn't make a lot of
>>> sense to me.
>>>
>>>
>>> What about an invalid UUID[0] being assigned with a valid UUID[16] or
>>> UUID[20]? Why doesn't this make sense? I don't follow.
>>>
>>
>> Nothing is invalid, I just think it's better and expresses the intent
>> more clearly if you can only assign between UUIDs of the same size.  For
>> example, If the UUID class were templated on size, then there would not
>> even be such thing as a UUID[0] or a "universally invalid UUID".  There
>> would be an "invalid 16-byte UUID" and an "invalid 20-byte UUID", and those
>> would be different things.
>>
>>
>>>
>>>
>>> As a future cleanup, I think this class should probably be a template
>>> such as UUID, and then internally it can store a std::array>> N>.  And we can static_assert that N is of a known size if we desire.
>>>
>>>
>>> UUID values are objects contained as members inside of other objects.
>>> They all default to start with no preconceived notion of what the UUID
>>> should be. IMHO the UUID class is just fine and needs to be able to
>>> represent any UUID, from empty uninitialized ones, and be able to be
>>> assigned and changed at will.
>>>
>>>
>> Is there ever a use case for changing the number of bytes in a UUID?  If
>> you're working with 16-byte UUIDs, does it ever actually happen that now
>> you have a 20-byte UUID?  Can you imagine a use case currently where an
>> N-byte UUID is being compared against an M-byte UUID in a real-world
>> scenario?  If the answer is no, then it may as well be enforced by the
>> compiler.
>>
>>
>> The ObjectFile class has a "UUID m_uuid;" member that any object file can
>> fill in. Right now mach-o files have 16 byte UUIDs. ELF files can have 20
>> bytes UUIDs (build ID) or 4 byte UUIDs (debug info CRC if no build ID is
>> around, and these are current represented as 20 byte UUIDs with just the
>> first 4 bytes filled in. So no, we can't enforce this using the compiler. I
>> don't see a need to change way from a byte buffer that has the max number
>> of bytes needed for any currently supported UUID (20 right now).
>>
>

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Zachary Turner via lldb-commits]

Eh, that actually just makes me think the compiler *can* check it.  For
example, right now you can have mach-o files with 20 byte UUIDs.  But just
in the code, not in practice.  You could have a bug in your code that
accidentally wrote the wrong number of bytes from a dynamic buffer.

You could enforce this at the compiler level by saying:

class ObjectFileMachO {
  UUID<16> uuid;
};

Not only is this more correct, but it is less error prone and is also nice
documentation to the reader who may be just learning about MachO that this
UUID is always 16 bytes.

For the case of ELF, it sounds like you either have a 20 byte UUID or a 4
byte UUID, but never both, and never any other size.  That makes me think
of:

class ObjectFileELF {
  union {
UUID<20> BuildId;
UUID<4> DebugCrc;
  }
};

And now the person reading this code can immediately tell that there will
either be one or the other, and depending on which one it is, he/she
already knows something about it, like how many bytes it is and what it
represents.

To me this is much more clear than:

class ObjectFileELF {
  // This might not actually be a build id, and it could be a variable
size, and you also have to be careful
  // not to put some strange number of bytes in it that we don't recognize,
but it's up to the user
  // to know under what circumstances it should be a certain number of
bytes, and you should also always
  // be careful ensure that there's no buffer overruns since you'll be
working with dynamically sized buffers
  // and the compiler can't warn you when you're doing something wrong.
  UUID BuildIdOrDebugCrc;
};

On Tue, Nov 28, 2017 at 11:06 AM Greg Clayton  wrote:

>
> On Nov 28, 2017, at 10:24 AM, Zachary Turner  wrote:
>
>
>
> On Tue, Nov 28, 2017 at 10:18 AM Greg Clayton  wrote:
>
>> On Nov 27, 2017, at 10:11 PM, Zachary Turner  wrote:
>>
>> As an aside, I don't really like this class.  For example, You can
>> currently assign a UUID[16] to a UUID[20].  That doesn't make a lot of
>> sense to me.
>>
>>
>> What about an invalid UUID[0] being assigned with a valid UUID[16] or
>> UUID[20]? Why doesn't this make sense? I don't follow.
>>
>
> Nothing is invalid, I just think it's better and expresses the intent more
> clearly if you can only assign between UUIDs of the same size.  For
> example, If the UUID class were templated on size, then there would not
> even be such thing as a UUID[0] or a "universally invalid UUID".  There
> would be an "invalid 16-byte UUID" and an "invalid 20-byte UUID", and those
> would be different things.
>
>
>>
>>
>> As a future cleanup, I think this class should probably be a template
>> such as UUID, and then internally it can store a std::array> N>.  And we can static_assert that N is of a known size if we desire.
>>
>>
>> UUID values are objects contained as members inside of other objects.
>> They all default to start with no preconceived notion of what the UUID
>> should be. IMHO the UUID class is just fine and needs to be able to
>> represent any UUID, from empty uninitialized ones, and be able to be
>> assigned and changed at will.
>>
>>
> Is there ever a use case for changing the number of bytes in a UUID?  If
> you're working with 16-byte UUIDs, does it ever actually happen that now
> you have a 20-byte UUID?  Can you imagine a use case currently where an
> N-byte UUID is being compared against an M-byte UUID in a real-world
> scenario?  If the answer is no, then it may as well be enforced by the
> compiler.
>
>
> The ObjectFile class has a "UUID m_uuid;" member that any object file can
> fill in. Right now mach-o files have 16 byte UUIDs. ELF files can have 20
> bytes UUIDs (build ID) or 4 byte UUIDs (debug info CRC if no build ID is
> around, and these are current represented as 20 byte UUIDs with just the
> first 4 bytes filled in. So no, we can't enforce this using the compiler. I
> don't see a need to change way from a byte buffer that has the max number
> of bytes needed for any currently supported UUID (20 right now).
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Greg Clayton via lldb-commits]

> On Nov 28, 2017, at 10:24 AM, Zachary Turner  wrote:
> 
> 
> 
> On Tue, Nov 28, 2017 at 10:18 AM Greg Clayton  > wrote:
>> On Nov 27, 2017, at 10:11 PM, Zachary Turner > > wrote:
>> 
>> As an aside, I don't really like this class.  For example, You can currently 
>> assign a UUID[16] to a UUID[20].  That doesn't make a lot of sense to me.
> 
> What about an invalid UUID[0] being assigned with a valid UUID[16] or 
> UUID[20]? Why doesn't this make sense? I don't follow.
> 
> Nothing is invalid, I just think it's better and expresses the intent more 
> clearly if you can only assign between UUIDs of the same size.  For example, 
> If the UUID class were templated on size, then there would not even be such 
> thing as a UUID[0] or a "universally invalid UUID".  There would be an 
> "invalid 16-byte UUID" and an "invalid 20-byte UUID", and those would be 
> different things.
>  
> 
>> 
>> As a future cleanup, I think this class should probably be a template such 
>> as UUID, and then internally it can store a std::array.  And 
>> we can static_assert that N is of a known size if we desire.
> 
> UUID values are objects contained as members inside of other objects. They 
> all default to start with no preconceived notion of what the UUID should be. 
> IMHO the UUID class is just fine and needs to be able to represent any UUID, 
> from empty uninitialized ones, and be able to be assigned and changed at will.
> 
> 
> Is there ever a use case for changing the number of bytes in a UUID?  If 
> you're working with 16-byte UUIDs, does it ever actually happen that now you 
> have a 20-byte UUID?  Can you imagine a use case currently where an N-byte 
> UUID is being compared against an M-byte UUID in a real-world scenario?  If 
> the answer is no, then it may as well be enforced by the compiler. 

The ObjectFile class has a "UUID m_uuid;" member that any object file can fill 
in. Right now mach-o files have 16 byte UUIDs. ELF files can have 20 bytes 
UUIDs (build ID) or 4 byte UUIDs (debug info CRC if no build ID is around, and 
these are current represented as 20 byte UUIDs with just the first 4 bytes 
filled in. So no, we can't enforce this using the compiler. I don't see a need 
to change way from a byte buffer that has the max number of bytes needed for 
any currently supported UUID (20 right now). ___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Zachary Turner via lldb-commits]

On Tue, Nov 28, 2017 at 10:18 AM Stephane Sezer via Phabricator <
revi...@reviews.llvm.org> wrote:

>
>
> The other alternative seems a bit less explicit to me but I don't mind it
> too much. What's the issue with using `std::any_of` exactly?
>
>
In the sense of correctness, nothing is wrong with using std::any_of.  But
if there's more than one way to do it, and one is easier to read /
understand, I just prefer it.  I'm not blocking the CL or anything, it was
just a suggestion in the interest of readability.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Zachary Turner via lldb-commits]

On Tue, Nov 28, 2017 at 10:24 AM Zachary Turner  wrote:

> On Tue, Nov 28, 2017 at 10:18 AM Greg Clayton  wrote:
>
>> On Nov 27, 2017, at 10:11 PM, Zachary Turner  wrote:
>>
>> As an aside, I don't really like this class.  For example, You can
>> currently assign a UUID[16] to a UUID[20].  That doesn't make a lot of
>> sense to me.
>>
>>
>> What about an invalid UUID[0] being assigned with a valid UUID[16] or
>> UUID[20]? Why doesn't this make sense? I don't follow.
>>
>
> Nothing is invalid, I just think it's better and expresses the intent more
> clearly if you can only assign between UUIDs of the same size.  For
> example, If the UUID class were templated on size, then there would not
> even be such thing as a UUID[0] or a "universally invalid UUID".  There
> would be an "invalid 16-byte UUID" and an "invalid 20-byte UUID", and those
> would be different things.
>
>
>>
>>
>> As a future cleanup, I think this class should probably be a template
>> such as UUID, and then internally it can store a std::array> N>.  And we can static_assert that N is of a known size if we desire.
>>
>>
>> UUID values are objects contained as members inside of other objects.
>> They all default to start with no preconceived notion of what the UUID
>> should be. IMHO the UUID class is just fine and needs to be able to
>> represent any UUID, from empty uninitialized ones, and be able to be
>> assigned and changed at will.
>>
>>
> Is there ever a use case for changing the number of bytes in a UUID?  If
> you're working with 16-byte UUIDs, does it ever actually happen that now
> you have a 20-byte UUID?  Can you imagine a use case currently where an
> N-byte UUID is being compared against an M-byte UUID in a real-world
> scenario?  If the answer is no, then it may as well be enforced by the
> compiler.
>

Also, if the data is just a `std::array`, and that size can never change,
then memcmps and finds are replaced with equality operators, which is a win
IMO.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Zachary Turner via lldb-commits]

On Tue, Nov 28, 2017 at 10:18 AM Greg Clayton  wrote:

> On Nov 27, 2017, at 10:11 PM, Zachary Turner  wrote:
>
> As an aside, I don't really like this class.  For example, You can
> currently assign a UUID[16] to a UUID[20].  That doesn't make a lot of
> sense to me.
>
>
> What about an invalid UUID[0] being assigned with a valid UUID[16] or
> UUID[20]? Why doesn't this make sense? I don't follow.
>

Nothing is invalid, I just think it's better and expresses the intent more
clearly if you can only assign between UUIDs of the same size.  For
example, If the UUID class were templated on size, then there would not
even be such thing as a UUID[0] or a "universally invalid UUID".  There
would be an "invalid 16-byte UUID" and an "invalid 20-byte UUID", and those
would be different things.


>
>
> As a future cleanup, I think this class should probably be a template such
> as UUID, and then internally it can store a std::array.  And
> we can static_assert that N is of a known size if we desire.
>
>
> UUID values are objects contained as members inside of other objects. They
> all default to start with no preconceived notion of what the UUID should
> be. IMHO the UUID class is just fine and needs to be able to represent any
> UUID, from empty uninitialized ones, and be able to be assigned and changed
> at will.
>
>
Is there ever a use case for changing the number of bytes in a UUID?  If
you're working with 16-byte UUIDs, does it ever actually happen that now
you have a 20-byte UUID?  Can you imagine a use case currently where an
N-byte UUID is being compared against an M-byte UUID in a real-world
scenario?  If the answer is no, then it may as well be enforced by the
compiler.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Greg Clayton via lldb-commits]

> On Nov 27, 2017, at 10:11 PM, Zachary Turner  wrote:
> 
> As an aside, I don't really like this class.  For example, You can currently 
> assign a UUID[16] to a UUID[20].  That doesn't make a lot of sense to me.

What about an invalid UUID[0] being assigned with a valid UUID[16] or UUID[20]? 
Why doesn't this make sense? I don't follow.

> 
> As a future cleanup, I think this class should probably be a template such as 
> UUID, and then internally it can store a std::array.  And we 
> can static_assert that N is of a known size if we desire.

UUID values are objects contained as members inside of other objects. They all 
default to start with no preconceived notion of what the UUID should be. IMHO 
the UUID class is just fine and needs to be able to represent any UUID, from 
empty uninitialized ones, and be able to be assigned and changed at will.

Greg

> 
> On Mon, Nov 27, 2017 at 9:38 PM Davide Italiano via Phabricator 
> > wrote:
> davide added a comment.
> 
> Yes, what Zachary said. Thanks for the cleanup. There's a decent amount of 
> code in lldb that can be written in a very compact way but it's manually 
> unrolled. I think in this case the code produced should be equivalent (and if 
> not, I'd be curious to see the codegen).
> More generally, whenever a function is not in a hot loop, we might consider 
> preferring readability over performances anyway.
> 
> 
> https://reviews.llvm.org/D40537 
> 
> 
> 

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

[Zachary Turner via lldb-commits]

As an aside, I don't really like this class.  For example, You can
currently assign a UUID[16] to a UUID[20].  That doesn't make a lot of
sense to me.

As a future cleanup, I think this class should probably be a template such
as UUID, and then internally it can store a std::array.  And
we can static_assert that N is of a known size if we desire.

On Mon, Nov 27, 2017 at 9:38 PM Davide Italiano via Phabricator <
revi...@reviews.llvm.org> wrote:

> davide added a comment.
>
> Yes, what Zachary said. Thanks for the cleanup. There's a decent amount of
> code in lldb that can be written in a very compact way but it's manually
> unrolled. I think in this case the code produced should be equivalent (and
> if not, I'd be curious to see the codegen).
> More generally, whenever a function is not in a hot loop, we might
> consider preferring readability over performances anyway.
>
>
> https://reviews.llvm.org/D40537
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits