On 28/03/2019 01:36, Jim Ingham wrote:
If you have a library that has a version number of 0.0.0,
uint32_t SBModule::GetVersion(uint32_t *versions, uint32_t num_versions)
will return a result of 2 (which is the number of elements it put into
num_versions) and the two elements it actually stuffed into the versions array
will be {UINT32_MAX, 0}.
That seems like a weird result to me, but the code goes to some trouble to make
this up. I was wondering what the reason for this is?
I think that is a bug. I am pretty sure my intention when writing this
code was to fill in everything with UINT32_MAX in this case, but I did
not implement that correctly. The code for computing the "result" was
probably meant to be something like:
---
result = 0;
if (!version.empty()) {
++result;
if(version.getMinor())
++result;
if(version.getSubminor())
++result;
}
---
which would give an all-UINT32_MAX array in this case (and avoid the
swig wrapper crash).
However, that doesn't mean we have to implement it that way..
So I was wondering whether the code in GetVersion that fills the Major version
with UINT32_MAX was done for some reason, or is just an accident I should fix.
The idea there was to have this signal that the Module has no version at
all. However, given that ObjectFileMachO is the only one who uses this
field, and the convention there seems to be to treat 0 as a valid
version (i.e., the binary format has no ability to express "I absolutely
don't know my version), changing that seems fine to me.
BTW, this comes about because llvm::VersionTuple doesn't have a HasMajor flag, and the
only way to test the validity of the major version is to call
"VersionTuple::empty" and that ONLY checks the values of the version numbers.
So something that has HasMinor = true but all the version numbers are 0 will say it is
empty - which also seems a little odd to me...
Yes there, seems to be some confusion around the usage of empty(). The
MachObjectWriter will serialize an empty VersionTuple as 0
<https://github.com/llvm-mirror/llvm/blob/master/lib/MC/MachObjectWriter.cpp#L874>,
but then there are also plenty of places which treat an empty
VersionTuple as invalid
<https://github.com/llvm-mirror/clang/blob/master/lib/AST/DeclBase.cpp#L546>.
(Incidentally, the MachObjectWriter stuff has to be fairly new code,
because VersionTuple was present only in clang until recently.)
Changing empty() to check the value of HasMinor flag sounds ok to me --
after all, if someone went to the trouble of spelling it out, he
probably really meant that value to be valid. However, it does create a
situation where it is possible to express the version "0.0", and "1",
but not "0", which may or may not be fine...
Overall, it might be better to add a HasMajor flag too, and have the
VersionTuple be invalid (empty) only when this flag is false. Or, we
could drop the empty() method altogether, and use Optional<VersionTuple>
to for the cases where the version is completely absent.
pl
_______________________________________________
lldb-dev mailing list
lldb-dev@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
