[mesa] des crashs avec le duo mesa 11.0.3 - llvm-libs 3.7.0

[Elbarto]

le rapport de bug LLVM a été crée il y a 2 semaines, il faut tenir compte du contexte du moment,

je pense que tu es très sévère

pour moi ce rapport est très utile pour un éventuel développeur, il présente les faits de manière très complète ( configuration, OS, les circonstances d'apparition du bug ) avec un moyen de reproduire le bug dans une machine virtuelle et même un workaround, il y a un backtrace, tout y est pour que ça fasse tilt dans la tête du développeur LLVM,

les mauvais rapports de bug c'est quand l'utilisateur donne peu d'informations sur sa configuration matérielle, genre il vient juste pour dire "ouin ça marche pas, help" en omettant de donner les indices utiles au développeur,

après je reconnais que je donne parfois trop d'informations ( surtout dans le rapport de bug de mesa où je me suis trop étalé en hypothèses ), il faut que je reste un peu plus dans le factuel sans donner d'hypothèses ( c'est au développeur de se décarcasser pour trouver la solution, il connait parfaitement son logiciel, alors que celui qui rapporte le bug n'est pas forcément développeur, et même s'il l'est il ne connait pas toujours les briques logiciels complexes utilisées par ce projet, son architecture )

[benjarobin]

Hum, je suis désolé, plate excuse, j'ai tout mélangé, le rapport de bug sur llvm est pas si mal que cela... Il manque plus que rajouter dans un nouveau commentaire une hypothèse lié au SSE4
Il m'est arrivé sur des projet de faire un rapport de bug avec le patch correctif, tout bien expliqué... Il a trainé pendant des semaines... heureusement une autre personne extérieure au projet à prit le temps de contacter les dev sur la mailing list pour faire incorporer le patch en question.

[Elbarto]

j'ai peut-être localisé l'endroit où se trouve le bug dans le code source de llvm 3.7.0 ( je préfère rester très prudent vu les hypothèses erronées ) :

c'est dans le fichier /lib/Support/Host.cpp

c'est là dedans que se trouve la fonction qui retourne le type de CPU, la classe "sys" qui a une fonction "getHostCPUName()'

Code : Tout sélectionner

StringRef sys::getHostCPUName()

un processeur est défini par une valeur "family", puis par son model et son stepping, dans dmesg je peux lire ça :

Code : Tout sélectionner

Intel Pentium(R) Dual-Core  CPU      E6800  @ 3.33GHz (fam: 06, model: 17, stepping: 0a)

donc c'est la famille "6" et le modèle 17, dans cette fonction getHostCPUName() le développeur determine la valeur "family", puis la valeur "model" à l'aide de la fonction DetectX86FamilyModel(), et fait ensuite un énorme "switch/case" imbriqué ( d'abord un switch sur "family" et dedans un switch sur le model ), il retourne ensuite un string ( Penryn, core2 etc... ) selon les combinaisons family/model,

il est possible qu'il ait merdé là dedans, en comparant avec la version 3.6.2 du fichier Host.cpp je vois qu'il y a des différences subtiles,

le code source de la version 3.6.2 est dispo ici :

http://llvm.org/releases/3.6.2/llvm-3.6.2.src.tar.xz

la 3.7.0 ici :
http://llvm.org/releases/3.7.0/llvm-3.7.0.src.tar.xz

ce serait bien si on pouvait trouver ensemble ce qui cloche dans la version 3.7.0 de ce fichier, afin qu'on puisse poster le patch sur le site de llvm, je sens qu'on est sur une très bonne piste,

je vais essayer de trouver des différences entre les 2 versions de ce fichier, puis modifier une ligne,

je vais aussi créer une version debug de llvm afin d'utiliser un débogueur pour y mettre des points d'arrêt dans cette fonction getHostCPUName() afin de vérifier le contenu de certaines variables, ça m'aidera à comprendre les choses,

rapidement vu comme ça ( j'ai pas encore débogué ) son switch/case est conçu bizarrement, il n'a pas mis tous les cas de figures pour le model ( les case xx ), il manque le case "17" de la family "6", ce qui fait qu'on passe par "default:" où il fait des tests sur "HasSSE41", si oui alors il retourne "Penryn" comme valeur

Code : Tout sélectionner

default: // Unknown family 6 CPU, try to guess.
        if (HasAVX512)
          return "knl";
        if (HasADX)
          return "broadwell";
        if (HasAVX2)
          return "haswell";
        if (HasAVX)
          return "sandybridge";
        if (HasSSE42)
          return HasMOVBE ? "silvermont" : "nehalem";
        if (HasSSE41)
          return "penryn";
        if (HasSSSE3)
          return HasMOVBE ? "bonnell" : "core2";
        if (Em64T)
          return "x86-64";
        if (HasSSE2)
          return "pentium-m";
        if (HasSSE)
          return "pentium3";
        if (HasMMX)
          return "pentium2";
        return "pentiumpro";
      }

le booléen HasSSE41 est défini comme ça dans la version 3.6.2 :

Code : Tout sélectionner

bool HasSSE41 = (ECX & 0x80000); 

dans la version 3.7.0 c'est différent :

Code : Tout sélectionner

bool HasSSE41 = (ECX >> 19) & 1;

il faudrait comprendre si ce changement est cohérent ou bien s'il conduit à une valeur erronée ( je suis pas très fort en décalage de bits )

une solution de facilité serait d'ajouter manuellement le "case 17 : return "core2";" dans le switch de la family 6, je verrai ça demain à tête reposée si mes hypothèses sont bonnes

[Elbarto]

en fait le 17 pour le modèle CPU renvoyé par dmesg est une valeur hexadécimale, ce qui en décimal équivaut à "23",

donc c'est pas "case 17: return "core2" mais plutôt "case 23: return "core2;"

l'erreur dans le code source se trouve alors ici ( on est toujours dans le switch (model) correspondant à la "family" 6 ) :

Code : Tout sélectionner

      case 23: // Intel Core 2 Extreme processor, Intel Xeon processor, model
               // 17h. All processors are manufactured using the 45 nm process.
               //
               // 45nm: Penryn , Wolfdale, Yorkfield (XE)
      case 29: // Intel Xeon processor MP. All processors are manufactured using
               // the 45 nm process.
        return "penryn";

le développeur considère que "family 6, model 23" est un penryn, alors que dans la version 3.6.2 il n'avait pas commis cette erreur car il faisait un test sur hasSSE41 pour différencier le penryn du core2 :

Code : Tout sélectionner

      case 23: // Intel Core 2 Extreme processor, Intel Xeon processor, model
               // 17h. All processors are manufactured using the 45 nm process.
               //
               // 45nm: Penryn , Wolfdale, Yorkfield (XE)
        // Not all Penryn processors support SSE 4.1 (such as the Pentium brand)
        return HasSSE41 ? "penryn" : "core2";

la bonne nouvelle si ma théorie se tient c'est que le booléen HasSSE41 n'est pas le fautif, ce serait alors juste un mauvais traitement du "case 23", je vais créer le patch, compiler et je viendrais donner des nouvelles

[Elbarto]

ça y est le bug est résolu grâce à mon patch :

Code : Tout sélectionner

--- a/lib/Support/Host.cpp    2015-10-14 07:13:52.381374679 +0200
+++ b/lib/Support/Host.cpp   2015-10-14 07:13:28.224708323 +0200
@@ -332,6 +332,8 @@
                // 17h. All processors are manufactured using the 45 nm process.
                //
                // 45nm: Penryn , Wolfdale, Yorkfield (XE)
+        // Not all Penryn processors support SSE 4.1 (such as the Pentium brand)
+        return HasSSE41 ? "penryn" : "core2";          
       case 29: // Intel Xeon processor MP. All processors are manufactured using
                // the 45 nm process.
         return "penryn"; 

j'ai juste rajouté ce test de manière à ce que le CPU family 6 model 23 ne soit pas systématiquement consideré comme un penryn, il devra avoir l'extension SSE4, sinon il sera consideré comme un core2 ( ce qui est le comportement de la version 3.6.2 ) :

Code : Tout sélectionner

return HasSSE41 ? "penryn" : "core2"; 

donc c'est un patch sans risque qui devrait pouvoir être accepté par les développeurs d'archlinux afin de proposer une révision 3.7.0-5 du paquet llvm et llvm-libs, et idéalement aussi par les développeurs de llvm,

l'idéal ce serait de trouver le commit qui a introduit ce bug, j'ai fait des recherches et je pense que le commit doit se trouver pas loin après celui du 30 mars 2015 :

Code : Tout sélectionner

r233514 | ctopper | 2015-03-30 08:31:03 +0200 (lun. 30 mars 2015) | 1 ligne
 
[X86] Family 6 model 29 is a Penryn based processor not a Nehalem based processor.

https://github.com/llvm-mirror/llvm/com ... 5b7a4ae69b

[Elbarto]

voici donc le commit qui a introduit ce bug :

https://github.com/llvm-mirror/llvm/com ... 2d27d1e54a

je vais publier cette trouvaille et ce patch sur le bugzilla de mesa, llvm et celui d'archlinux

[benjarobin]

Le commit que tu cites est juste : https://github.com/llvm-mirror/llvm/com ... 2d27d1e54a
Avant c'était juste moche... Il corrige bien le nom de la famille du CPU (ton CPU est bien de la famille penryn), par contre en effet il introduit un bug. Il faut penser à lire le commentaire du commit...
Ton patch ne fait qu'annuler ce commit, ce qui est acceptable comme moyen de contournement, mais clairement pas la solution définitive.

[Elbarto]

je me suis surtout occupé de son switch/case de la fonction getHostCPUName(), car c'est là dedans qu'il retourne le nom du CPU en fonction du couple "family/model",

du coup j'ai pas vraiment compris pourquoi il a effacé le test " return HasSSE41 ? "penryn" : "core2";" dans ce switch/case sans le remplacer par un test équivalent,

ou alors on lui a dit que les CPU "family 6 model 23" ont tous le SSE4 ce qui rend inutile le test, là je pourrais comprendre le fait qu'il ait supprimé le test

mais bon s'il était un peu plus méfiant et rigoureux il aurait dû essayer de vérifier cette information ( wikipédia, des tests auprès de possesseurs de pentium dual core )

grosso modo je trouve que sa fonction "StringRef sys::getHostCPUName()" est un peu fouillis, il y a beaucoup de lignes de code, on m'a toujours dit en cours de programmation que lorsqu'une fonction devenait un gros pâté c'est que le design n'était pas bon, il faudrait factoriser le code ou créer d'autres fonctions pour y reporter des lignes de codes afin d’alléger l'ensemble, pour que ça soit plus lisible,

la partie où il calcule les booléens hasXXX devrait être déportée dans une autre fonction, c'est peut-être le but des fonctions getHostCPUFeatures() dont parle le texte du commit, mais bizarrement il ne les utilise pas dans le switch/case du getHostCPUName(), il n'a fait que supprimer les tests sur AVX

[benjarobin]

C'est tout sauf fouillis, je ne vois pas un patté, après on aime ou pas ce type d'indentation... C'est bien commenté en plus. Toi tu n'as jamais fait de développement...
As tu lu le message de commit ou mes messages ?

Code : Tout sélectionner

 [X86] Stop changing result of getHostCPUName based on whether the processor supports AVX. getHostCPUFeatures should be used instead to determine whether to support AVX.

Ton CPU est bien de la famille Penryn !!!! Il doit bien afficher ceci !
Avant c'était un contournement très moche, le commit corrige correctement la chose...
Ce qui est fait est juste, mais incomplet... Très certainement getHostCPUFeatures() ne doit pas être utilisé correctement, ou pire c'est cette fonction qui est fausse.

[Elbarto]

Ok, j'espère qu'ils trouveront un meilleur correctif

[benjarobin]

Dans tous les cas tu as fait 99% du travail (bien que tes conclusions soient en parties erronées), merci pour cela, je suis sûre que plein de personne pourrait te remercier. Ils ont toutes les informations pour corriger la chose.

[Elbarto]

une question ( ) :

est-on sûr à 100% qu'il s'agit vraiment d'un bug de llvm ?

après tout c'est peut-être mesa qui utilise mal la lib llvm, mesa a la possibilité de forcer l'usage d'extensions SSE d'après ce que je comprends ici :

http://lists.freedesktop.org/archives/m ... 68957.html

ce que je crains qu'en fait il n'y ait jamais eu de bug llvm à proprement parlé, mesa pourrait avoir profité du test généreux " HasSSE41 ? "penryn" : "core2" qui a perduré jusqu'à la version 3.6.2, ce qui peut avoir exempté mesa de faire des vérifs de sécurité pour le cas de la famille penryn,

un bug de mesa longtemps camouflé par le fait qu'il reçevait un core2 au lieu d'un penryn, ce cadeau s'est arrêté depuis la version 3.7.0, pas d'adaptation du code source de mesa = bug qui apparait au grand jour

je ne sais pas si je suis clair ( je fais tellement d'hypothèses )

[Elbarto]

bon oubliez ma dernière hypothèse, on va laisser les choses comme ça, les développeurs llvm devraient pouvoir finaliser tout ça, inutile de se casser la tête dessus

en attendant voici le PGKBUILD modifié avec le patch ( et ceux déjà présents dans la version officielle 3.7.0-4 ) :

http://demo.ovh.eu/download/56098cafcbc ... 0-5.tar.gz

ça sera utile pour Bruno s'il veut construire lui-même les paquets llvm

[benjarobin]

Non c'est loin d'être bête. Tu as peut être raison. Mais normalement mesa devrait utiliser getHostCPUFeatures pour savoir si SSE4 est disponible. Je n'ai toujours pas pris le temps de regarder comment tout ceci s'agence

[Elbarto]

il semblerait que dans le code source de mesa 11.0.3 il n'y ait aucune utilisation de la fonction "getHostCPUFeatures",

je n'ai trouvé que l'utilisation de la fonction "getHostCPUName" dans le fichier /src/gallium/auxiliary/gallivm/lp_bld_misc.cpp,
ensuite mesa possède un fichier "/src/gallium/auxiliary/utilu_cpu_detect.c" où il fait des détections sur les fonctionnalités du CPU,

ce qui m'intrigue c'est que dans le code source de llvm-3.7.0 il y a un fichier qui liste les noms de CPU en leur associant des "features" type SSE2, SSE3 etc, ce fichier porte un nom étrange "/lib/Target/X86.td", c'est la même liste de noms qu'on trouve dans le fichier Host.cpp :

Code : Tout sélectionner

// Intel Core 2 Solo/Duo.
def : ProcessorModel<"core2", SandyBridgeModel,
                     [FeatureSSSE3, FeatureCMPXCHG16B, FeatureSlowBTMem]>;
def : ProcessorModel<"penryn", SandyBridgeModel,
                     [FeatureSSE41, FeatureCMPXCHG16B, FeatureSlowBTMem]>;

je me demande à quoi il sert, car c'est troublant ce qu'il y a dedans,

le X86.td est peut-être utilisé automatiquement par un composant de llvm qui génère du code machine en fonction d'une cible CPU, genre on lui donne le nom du CPU et il va vérifier les features disponibles dans X86.td

comme dans ce fichier il n'y a qu'une seule définition des features de penryn ( avec le SSE4 activé ) ça posera problème si le penryn était en fait un pentium dual core Exxxx type wolfdale et que mesa n'a pas fait de vérification via "getHostCPUFeatures()",

à creuser tout ça à tête reposée...

[brunoo]

ça sera utile pour Bruno s'il veut construire lui-même les paquets llvm

Merci à toi

En espérant que ce bug soit tout de même résolu pour les prochaines versions.

[benjarobin]

J'ai pris quelques minutes pour regarder en détail. Tout d'abord je pense que le bug est dans llvm, je en vois pas comment c'est possible qu'il soit dans mesa.
De plus de ce que j'ai pu comprendre c'est que le getHostCPUName() n'était utilisé que pour récupérer les infos dans X86.td... Et c'est là où je ne comprend plus rien, enfin je ne comprends donc plus du tout le commit, et encore moins son message (de commit)... Pour moi l'intégralité de ce commit devrait être annulé !
Mais c'est tellement complexe que j'avoue ne pas tout comprendre...

[Elbarto]

oui c'est aussi un peu mon sentiment, un commit bizarre, ou alors ce commit ne visait qu'à retirer le test sur les extensions AVX dans la fonction getHostCPUName(), c'est ce terme AVX qui apparait dans le commentaire du commit, aucune référence n'est faite au SSE4, l'auteur du commit en a peut-être fait trop, excès de zèle en supprimant tout type de test dans les "return" :

Code : Tout sélectionner

 [X86] Stop changing result of getHostCPUName based on whether the processor supports AVX. getHostCPUFeatures should be used instead to determine whether to support AVX.

la question sur le réel but de ce commit a été d'ailleurs posée aujourd'hui par Foutras ( mainteneur archlinux du paquet llvm ) à l'auteur de ce commit dans le rapport de bug llvm

[Elbarto]

toujours pas de réponse de Craig Topper à la question de Foutras sur le rapport de bug llvm, c'est à se demander si les développeurs llvm consultent vraiment leur bugzilla

à priori ils sont planqués dans leur mailing-list de développeurs, ça me fait penser à ce que tu as dis plus haut, il faudra peut-être aller les chercher là-bas

http://lists.llvm.org/pipermail/llvm-de ... /date.html

il existe aussi une mailing-list llvm dédiée aux bugs llvm, automatiquement dès qu'on crée un rapport de bug un mail est envoyé à cette mailing-list :

http://lists.llvm.org/pipermail/llvm-bu ... /date.html

[Elbarto]

ne voyant pas de réaction des développeurs llvm sur le rapport de bug je me suis inscrit sur la mailing list llvm-dev pour mettre en avant ce bug,

le Cooper responsable du commit m'a repondu qu'il faut utiliser la fonction "getHostCPUFeatures()" pour savoir si le cpu supporte sse41 :

Craig Topper a écrit :
> That check should not be needed because getHostCPUFeatures() should also
> be called and detect that SSE41 is not supported. This should then pass
> "-sse41" into the feature selection.

je lui ai répondu que les développeurs mesa n'utilisaient pas cette fonction getHostCPUFeatures(), et qu'il existait un fichier /lib/Target/X86.td qui affectait des "features" d'office à la liste de nom CPU qu'on trouve dans le switch présent dans /lib/Support/Host.cpp, "penryn" a par défaut la feature "SSE41", d'où peut-être le bug,

il se peut que llvm et mesa se renvoient la patate chaude,

les CPU récents ayant tous le SSE41 je sens que personne ne va lever le petit doigt pour s'occuper des propriétaires de pentium dual core