> On Mar 28, 2019, at 3:03 AM, Pavel Labath <pa...@labath.sk> wrote:
>
> 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.
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
> pl
_______________________________________________
lldb-dev mailing list
lldb-dev@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
