[LNT] Fixed incorrect orders behavior in case of miss formatted llvm_project_revision

[kpdev42]

Summary

LNT expected that llvm_project_revision is a version in the format 1.2.3. But it is a string and may contain any symbols. Someone may use hex numbers, dashes, etc. If llvm_project_revision does not contain any digit at all the behavior is undefined and caused inconsistent DB. hash(str) is a workaround to fix this behavior. Note it does not fix the sorting.

[thopre]

If the goal is to prevent internal error when the version is not made of the expected dotted version, I'd rather LNT error out when importing a report if the version is invalid.

[kpdev42]

This code wont change previous behavior for versions which contains digits.
In this commit it is intended to prevent a situation when DB became silently inconsistent if llvm_project_revision does not contain any digit.
e.g: "alpha", "beta", "private", "main", "test", etc

[thopre]

In D109577#3003389, @kpdev42 wrote:

This code wont change previous behavior for versions which contains digits.
In this commit it is intended to prevent a situation when DB became silently inconsistent if llvm_project_revision does not contain any digit.
e.g: "alpha", "beta", "private", "main", "test", etc

My point is that llvm_project_revision should only contain digits and dots. We should give an error if that's not the case rather than trying to avoid DB inconsistencies.

[cmatthews]

One real problem that we are having is that the most common submission format that I know if is the git describe output. We are using that everywhere.

The numeric part is really what we want from that, but folks seem to just shoot the whole string in. Totally breaks the graphs and all sorts of stuff.

I was thinking about special casing that somehow.

<well-know-tag>-123456-HHHHHHHHHH

[slydiman]

Today I worked with the LNT instance which is configured to generate reports (format_version=1) with run_order like "76543-toolchain-default-heuristic-change-09-13-2021-14:59-392". The limitation to digits and dots will break too much. It is not an option.

Note the class Order calls convert_revision() from _get_comparison_discriminant(). _get_comparison_discriminant() is used in many places including eq().
So two different orders without digits in llvm_project_revision will be wrongly considered equivalent.
index = orders.index(order) inside _getOrCreateOrder() will return incorrect index too.
This patch fixes these issues.
I think this patch is safe and useful.

[thopre]

Hadn't thought of the git describe case, that would lead to a correct ordering indeed. However if llvm_revision_version is a hash the resulting ordering of runs would be completely useless. That's why I'd rather LNT refusing any revision that's not a version number so that the user has to make an inform decision on how to convert to a version number. For instance for "llvmorg-14-init-4025-g005fc11ebdd6" the user could convert to 14.4025 while for "abc" user could decide to convert to 1.2.3 (letter replaced by its position in alphabet, or by its ascii code). @cmatthews feel free to override me, but I think this will create silent problems, even though it might make some usecase easier (git describe case you mentioned)

[cmatthews]

Yep, this looks good then.

We will have to tackle the larger issue of invalid input though.

[cmatthews]

Actually, I just realized this has a serious bug. The hash function in python is salted, so results are different between process restarts.

https://docs.python.org/3/reference/datamodel.html#object.__hash__

Can you use a different hash function?

[kpdev42]

In D109577#3355478, @cmatthews wrote:

Actually, I just realized this has a serious bug. The hash function in python is salted, so results are different between process restarts.

https://docs.python.org/3/reference/datamodel.html#object.__hash__

Can you use a different hash function?

Could you please advice a preferable hash function implementation?

Note: I just want to clarify, that this solution (hash(str)) is a workaround to fix a problem in case when llvm_project_revision does not contain any digit. It did not intend to fix the sorting.

[danilaml]

hashlib should have deterministic hash functions.

[cmatthews]

Yep, I normally use hashlib as well.

[kpdev42]

Add hashlib

[cmatthews]

Wonderful! Thanks

lnt/server/ui/util.py

 import colorsys
+import hashlib
 import math
 import re
 from lnt.server.reporting.analysis import REGRESSED
 
 from flask import g
 
 
 def toColorString(col):

 integral_rex = re.compile(r"[\d]+")
 
 
 def convert_revision(dotted, cache=None):
     """Turn a version number like 489.2.10 into something
     that is ordered and sortable.
     "1" -> (1)
     "1.2.3" -> (1,2,3)
+    "1a2,3-45:b6;" -> (1,2,3,45,6)
+    "abc" -> (hash("abc"))
 
     :param dotted: the string revision to convert
     :param cache: a dict to use as a cache or None for no cache.
         because this is called many times, it is a nice performance
         increase to cache these conversions.
     :return: a tuple with the numeric bits of this revision as ints.
-
+        return a hash in case of miss formatted version to avoid wrong equals.
     """
     if cache is not None:
         val = cache.get(dotted)
         if val:
             return val
-        else:
-            dotted_parsed = integral_rex.findall(dotted)
-            val = tuple([int(d) for d in dotted_parsed])
-            cache[dotted] = val
-            return val
-    dotted_parsed = integral_rex.findall(dotted)
-    val = tuple([int(d) for d in dotted_parsed])
-    return val
+
+    val = tuple([int(d) for d in dotted_parsed] or 
+                [int(hashlib.sha1(dotted.encode("utf-8")).hexdigest(), 16)])
+    if cache is not None:
 
 
 class PrecomputedCR():
     """Make a thing that looks like a comprison result, that is derived
     from a field change."""
     previous = 0
     current = 0
     pct_delta = 0.00