What is the correct behavior of SBModule::GetVersion with a version number of 0.0.0

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?

This came to my attention because the swig wrapper for GetVersion treats UINT32_MAX as the sign that it should stop filling elements in the list array (even if the element that is UINT32_MAX is earlier in the returned array than the result that GetVersion returns.) That is, we do:

    uint32_t count = result;
    if (count >= arg3)
    count = arg3;
    PyObject* list = PyList_New(count);
    for (uint32_t j = 0; j < count; j++)
    {
      if (arg2[j] < UINT32_MAX)
      {
        PyObject* item = PyInt_FromLong(arg2[j]);
        int ok = PyList_SetItem(list,j,item);
        if (ok != 0)
        {
          resultobj = Py_None;
          break;
        }
      }
      else
      break;
    }

And so if the 0th element in the list in UINT32_MAX, we make a Python list that we say we are going to fill with two elements, but don't put anything in it, and so if you try to iterate over this list Python crashes.

The wrapper behavior seems clearly wrong. If you say you are making an array with N elements, you need to put N elements in it or iterating over it will crash. That's easy to fix.

But then if we have a library whose actual version (e.g. as printed by the llvm objdump) is 0.0.0, we will call it {4294967295, 0} which just seems weird.

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.

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...

Jim

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:

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.

For dylibs, the current_version field in the LD_ID_DYLIB load command (which is what we report in GetVersion) is always present, so there will always be a version number for mach-o dylibs. If you don't specify a version ld will fill it with zero. However, you can also specify -current_version 0.0.0 and that will produce exactly the same load command. So I don't think it is possible to meaningfully have "no version" for dylibs on mach-o. OTOH executables have no version number - presumably because the version number is for checking capabilities in a loaded object - and in that case the VersionTuple returned from the MachO plugin is not only "empty()" but HasMinor and HasSubminor are false. This properly produces an empty list return from the MachO object file plugin.

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.

Thanks for the explanation! When I get back to this I'll fix the SBModule::GetVersion for the 0.0 case and see about fixing the underlying behavior. Once we don't crash the odd printing is a minor issue, so I'm out of time for this right now.

Jim