[lldb-dev] What is the correct behavior of SBModule::GetVersion with a version number of 0.0.0

[Pavel Labath via lldb-dev]

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