toolchain sanity checks, revised

This introduces a sanity check for the toolchain, which verifies each tuning (including any multilibs), producing meaningful diagnostics for problems, and also provides some higher-level tuning features.

The TUNEVALID and TUNECONFLICT/TUNECONFLICTS settings were not implemented, and there were some loose ends (like not knowing how the conflict one was spelled). Listed one or two missing features in TUNEVALID, also (in a previous patch) fixed the references to features which didn't exist.

This patch also provides a whitelisting mechanism (which is completely unused) to allow vendors providing prebuilt toolchain components to restrict tunings to those based on or compatible with a particular ABI.

Signed-off-by Peter Seebach<[email protected]>

---
  meta/classes/sanity.bbclass                      |   67 ++++++++++++++++++++++
  meta/conf/documentation.conf                     |    6 ++
  meta/conf/machine/include/README                 |    4 +
  meta/conf/machine/include/arm/arch-armv5-dsp.inc |    1 +
  meta/conf/machine/include/arm/arch-armv7a.inc    |    2 +-
  meta/conf/machine/include/ia32/arch-ia32.inc     |    2 +-
  meta/conf/machine/include/mips/arch-mips.inc     |    6 +-
  meta/conf/machine/include/tune-c3.inc            |    2 +-
  8 files changed, 84 insertions(+), 6 deletions(-)

meta/classes/sanity.bbclass +12 -0

index 687ddeb..c77e675 100644
@@ -11,6 +11,70 @@ def raise_sanity_error(msg):
 
      %s""" % msg)
 
+# Check a single tune for validity.
+def check_toolchain_tune(data, tune, multilib):
+    tune_errors = []
+    if not tune or tune == "":
+        return "No tuning found for %s multilib." % multilib
+    bb.note("Sanity-checking tuning '%s' (%s) features:" % (tune, multilib))
+    features = data.getVar("TUNE_FEATURES_tune-%s" % tune, True) or ""
+    if features == '':
+        return "Tuning '%s' has no defined features, and cannot be used." % tune
+    features = features.split()
+    valid_tunes = data.getVarFlags('TUNEVALID') or ""
+    conflicts = data.getVarFlags('TUNECONFLICTS') or ""

meta/classes/sanity.bbclass +45 -0

+            tune_errors.append("Feature '%s' is not defined." % feature)
+    whitelist = data.getVar("TUNEABI_WHITELIST", True) or ''
+    if whitelist != '':
+        override = data.getVar("TUNEABI_OVERRIDE", True) or ''
+        if not override:
+            tuneabi = data.getVar("TUNEABI_tune-%s" % tune, True) or ""
+            if tuneabi == "":
+                tuneabi = tune
+            if True not in [x in whitelist.split() for x in tuneabi.split()]:
+                tune_errors.append("Tuning '%s' (%s) cannot be used with any supported tuning/ABI." %
+                    (tune, tuneabi))
+    if tune_errors:
+        return "Tuning '%s' has the following errors:\n" + '\n'.join(tune_errors)
+
+def check_toolchain(data):
+    tune_error_set = []
+    deftune = data.getVar("DEFAULTTUNE", True)
+    tune_errors = check_toolchain_tune(data, deftune, 'default')
+    if tune_errors:
+        tune_error_set.append(tune_errors)
+
+    multilibs = data.getVar("MULTILIBS", True) or ""
+    if multilibs != "":
+        for pairs in [x.split(':') for x in multilibs.split()]:
+            if pairs[0] != 'multilib':
+                bb.warn("Got an unexpected '%s' in MULTILIBS." % pairs[0])
+            else:
+                if pairs[1] == 'lib':
+                    tune_error_set.append("The multilib 'lib' was specified, but that causes parse errors.")
+                else:
+                    tune = data.getVar("DEFAULTTUNE_virtclass-multilib-%s" % pairs[1], True)
+                    if tune == deftune:
+                        bb.warn("Multilib '%s' (%s) is also the default tuning." % (pairs[1], deftune))
+                    else:
+                        tune_errors = check_toolchain_tune(data, tune, pairs[1])
+                    if tune_errors:
+                        tune_error_set.append(tune_errors)
+    if tune_error_set:
+        return "Toolchain tunings invalid:\n" + '\n'.join(tune_error_set)
+
+    return ""
+
  def check_conf_exists(fn, data):
      bbpath = []
      fn = data.expand(fn)

@@ -327,6 +391,9 @@ def check_sanity(e):
          messages = messages + pseudo_msg + '\n'
 
      check_supported_distro(e)
+    toolchain_msg = check_toolchain(e.data)
+    if toolchain_msg != "":
+        messages = messages + toolchain_msg + '\n'
 
      # Check if DISPLAY is set if IMAGETEST is set
      if not data.getVar( 'DISPLAY', e.data, True ) and data.getVar( 'IMAGETEST', e.data, True ) == 'qemu':

meta/conf/documentation.conf +6 -0

index f4d6241..99c16fb 100644
@@ -34,6 +34,12 @@ TARGET_CC_ARCH[doc] = "FIXME"
  TARGET_FPU[doc] = "Floating point option (mostly for FPU-less systems), can be 'soft' or empty \
  for hardware floating point instructions."
 
+# WARNING:  Because the flags on these have semantic implecations,
+# they must not actually be defined.
+#TUNEVALID[doc] = "Descriptions of valid tuning features, stored as flags."
+#TUNECONFLICTS[doc] = "List of conflicting features for a given feature."
+TUNEABI[doc] = "A base ABI used by a given tuning, used with prebuilt binaries."
+
  ASSUME_PROVIDED[doc] = "List of packages (recipes actually) which are assumed to be implicitly available.\
   These packages won't be built by bitbake."
  ASSUME_SHLIBS[doc] = "List of shlib:package[_version] mappings. Useful for lib packages in ASSUME_PROVIDED,\

meta/conf/machine/include/README +4 -0

index 6a3a63d..e4b59c9 100644
@@ -24,6 +24,10 @@ TUNEVALID[feature] - The<feature>  is defined with a human readable
  explanation for what it does.  All architectural, cpu, abi, etc tuning
  features must be defined using TUNEVALID.
 
+TUNECONFLICTS[feature] - A list of features which conflict with<feature>.
+New sanity checks will try to reject combinations in which a single
+tuning ends up with features which conflict with each other.
+
  TUNE_FEATURES - This is automatically defined as TUNE_FEATURES_tune-<tune>.
  See TUNE_FEATURES_tune-<tune>  for more information.
 

meta/conf/machine/include/arm/arch-armv5-dsp.inc +1 -0

index 9f03a0f..0f64562 100644
@@ -1,4 +1,5 @@
  ARMPKGSFX_DSP = "${@bb.utils.contains("TUNE_FEATURES", [ "armv5", "dsp" ], "e", "", d)}"
+TUNEVALID[dsp] = "ARM DSP functionality"
 
  require conf/machine/include/arm/arch-armv5.inc
 

meta/conf/machine/include/arm/arch-armv7a.inc +1 -1

index 629960d..c90aff5 100644
@@ -2,7 +2,7 @@ DEFAULTTUNE ?= "armv7a"
 
  ARMPKGARCH ?= "armv7a"
 
-TUNEVALID[armv7-a] = "Enable instructions for ARMv7-a"
+TUNEVALID[armv7a] = "Enable instructions for ARMv7-a"
  TUNE_CONFLICTS[armv7a] = "armv4 armv5 armv6 armv7"
  TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "armv7a", "-march=armv7-a -fno-tree-vectorize", "", d)}"
  MACHINEOVERRIDES .= "${@bb.utils.contains("TUNE_FEATURES", "armv7a", ":armv7a", "" ,d)}"

meta/conf/machine/include/ia32/arch-ia32.inc +1 -1

index a5dae88..15f67d7 100644
@@ -27,7 +27,7 @@ TUNE_ASARGS += "${@bb.utils.contains("TUNE_FEATURES", "mx32", "-x32", "", d)}"
 
  # ELF64 ABI
  TUNEVALID[m64] = "IA32e (x86_64) ELF64 standard ABI"
-TUNECONFLICT[m64] = "m32 mx32"
+TUNECONFLICTS[m64] = "m32 mx32"
  TUNE_ARCH .= "${@bb.utils.contains("TUNE_FEATURES", "m64", "${X86ARCH64}", "" ,d)}"
  TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "m64", "-m64", "", d)}"
 

meta/conf/machine/include/mips/arch-mips.inc +3 -3

index 8758ecd..9f12920 100644
@@ -12,15 +12,15 @@ TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "bigendian", "-meb", "-mel
 
  # ABI flags
  TUNEVALID[o32] = "MIPS o32 ABI"
-TUNECONFLICT[o32] = "n32 n64"
+TUNECONFLICTS[o32] = "n32 n64"
  TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "o32", "-mabi=32", "", d)}"
 
  TUNEVALID[n32] = "MIPS64 n32 ABI"
-TUNECONFLICT[n32] = "o32 n64"
+TUNECONFLICTS[n32] = "o32 n64"
  TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "n32", "-mabi=n32", "", d)}"
 
  TUNEVALID[n64] = "MIPS64 n64 ABI"
-TUNECONFLICT[n64] = "o32 n32"
+TUNECONFLICTS[n64] = "o32 n32"
  TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "n64", "-mabi=64", "", d)}"
 
  # Floating point

meta/conf/machine/include/tune-c3.inc +1 -1

index 06fac8f..79bb67b 100644
@@ -1,7 +1,7 @@
  require conf/machine/include/ia32/arch-ia32.inc
 
  TUNEVALID[c3] = "VIA Cyrix III or VIA C3 specific optimizations"
-TUNECONFLICT[c3] = "m64 mx32"
+TUNECONFLICTS[c3] = "m64 mx32"
  TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "c3", "-march=c3 -mtune=c3", "", d)}"
 
  AVAILTUNES += "c3"

Is the above debugging? (I suspect it is) I suggest the following...

Quoted 1 line · On Mon, 30 Apr 2012 15:42:44 -0500 Mark Hatle <[email protected]> wrote:

Is the above debugging? (I suspect it is) I suggest the following...

Actually, that one's intentional. I originally did it for debugging, but I thought it was super convenient to actually get a list of the CPU feature set bitbake thought it was using. We can't tell you whether you picked the right features, but we can tell you what they are.

So for instance, with a qemux86 and a core2 lib32:

NOTE: Sanity-checking tuning 'x86-64' (default) features:
NOTE:   checking for conflicts: m64
NOTE:   checking for conflicts with: m32
NOTE:   m64: IA32e (x86_64) ELF64 standard ABI
NOTE: Sanity-checking tuning 'core2' (lib32) features:
NOTE:   m32: IA32 ELF32 standard ABI
NOTE:   core2: Enable core2 specific processor optimizations

I found this really handy, and left it there on purpose. I could take it out, and/or move it to somewhere else, but I really do like having that information appear.

Quoted 20 lines

Quoted 1 line · On Mon, 30 Apr 2012 15:42:44 -0500 Mark Hatle <[email protected]> wrote:

Is the above debugging? (I suspect it is) I suggest the following...

Actually, that one's intentional. I originally did it for debugging, but I thought it was super convenient to actually get a list of the CPU feature set bitbake thought it was using. We can't tell you whether you picked the right features, but we can tell you what they are.

So for instance, with a qemux86 and a core2 lib32:

NOTE: Sanity-checking tuning 'x86-64' (default) features:
NOTE:   checking for conflicts: m64
NOTE:   checking for conflicts with: m32
NOTE:   m64: IA32e (x86_64) ELF64 standard ABI
NOTE: Sanity-checking tuning 'core2' (lib32) features:
NOTE:   m32: IA32 ELF32 standard ABI
NOTE:   core2: Enable core2 specific processor optimizations

Quoted 3 lines

I found this really handy, and left it there on purpose. I could take it out, and/or move it to somewhere else, but I really do like having that information appear.

I'd change these to debug messages. Users don't need this on the console on every run. There is already a summary of the active tune options displayed in the build configuration banner if I remember correctly.

TUNE_FEATURES = "armv7a vfp neon cortexa8"

This introduces a sanity check for the toolchain, which verifies each tuning (including any multilibs), producing meaningful diagnostics for problems, and also provides some higher-level tuning features.

The TUNEVALID and TUNECONFLICT/TUNECONFLICTS settings were not implemented, and there were some loose ends (like not knowing how the conflict one was spelled). Listed one or two missing features in TUNEVALID, also (in a previous patch) fixed the references to features which didn't exist.

This patch also provides a whitelisting mechanism (which is completely unused) to allow vendors providing prebuilt toolchain components to restrict tunings to those based on or compatible with a particular ABI.

Signed-off-by Peter Seebach <[email protected]>

---
 meta/classes/sanity.bbclass                      |   67 ++++++++++++++++++++++
 meta/conf/documentation.conf                     |    6 ++
 meta/conf/machine/include/README                 |    4 +
 meta/conf/machine/include/arm/arch-armv5-dsp.inc |    1 +
 meta/conf/machine/include/arm/arch-armv7a.inc    |    2 +-
 meta/conf/machine/include/ia32/arch-ia32.inc     |    2 +-
 meta/conf/machine/include/mips/arch-mips.inc     |    6 +-
 meta/conf/machine/include/tune-c3.inc            |    2 +-
 8 files changed, 84 insertions(+), 6 deletions(-)

meta/classes/sanity.bbclass +0 -0

index 687ddeb..c77e675 100644

meta/classes/sanity.bbclass +5 -0

+    if tune_errors:
+        return "Tuning '%s' has the following errors:\n" + '\n'.join(tune_errors)
+
+def check_toolchain(data):
+    tune_error_set = []

meta/classes/sanity.bbclass +12 -0

+                    else:
+                        tune_errors = check_toolchain_tune(data, tune, pairs[1])
+                    if tune_errors:
+                        tune_error_set.append(tune_errors)
+    if tune_error_set:
+        return "Toolchain tunings invalid:\n" + '\n'.join(tune_error_set)
+
+    return ""
+
 def check_conf_exists(fn, data):
     bbpath = []
     fn = data.expand(fn)

@@ -327,6 +391,9 @@ def check_sanity(e):
         messages = messages + pseudo_msg + '\n'
 
     check_supported_distro(e)
+    toolchain_msg = check_toolchain(e.data)
+    if toolchain_msg != "":
+        messages = messages + toolchain_msg + '\n'
 
     # Check if DISPLAY is set if IMAGETEST is set
     if not data.getVar( 'DISPLAY', e.data, True ) and data.getVar( 'IMAGETEST', e.data, True ) == 'qemu':

meta/conf/documentation.conf +0 -0

index f4d6241..99c16fb 100644

Quoted 4 lines

This patch also provides a whitelisting mechanism (which is completely unused) to allow vendors providing prebuilt toolchain components to restrict tunings to those based on or compatible with a particular ABI.

Quoted 1 line

+ if not tune or tune == "":

"if not tune:"

We'd usually have

features = (data.getVar("TUNE_FEATURES_tune-%s" % tune, True) or
"").split() if not features:
return "Tuning '%s' has no defined features, and cannot be
used." % tune

meta/classes/sanity.bbclass def raise_sanity_error(msg): · line 25

Quoted 2 lines

meta/classes/sanity.bbclass +2 -0

+    valid_tunes = data.getVarFlags('TUNEVALID') or ""
+    conflicts = data.getVarFlags('TUNECONFLICTS') or ""

Hmm, shouldn't this be "or {}" ?

Quoted 1 line

+ whitelist = data.getVar("TUNEABI_WHITELIST", True) or ''

So what is TUNEABI_WHITELIST?

and what is TUNEABI_OVERRIDE? These should probably get the [doc] tags you add for other things.

To be honest I'm really tempted to split this tuneabi stuff out into a different class. I appreciate the OSVs need something but it probably doesn't make sense with half an implementation sitting in the core like this, particularly when the code is near impenetrable like the above. I'm left wondering what TUNEABI is too...

Quoted 2 lines

+ bb.warn("Got an unexpected '%s' in MULTILIBS." %
pairs[0])

Shouldn't this get added to the errors list?

Quoted 4 lines

meta/classes/sanity.bbclass +3 -0

+            else:
+                if pairs[1] == 'lib':
+                    tune_error_set.append("The multilib 'lib' was

specified, but that causes parse errors.")

Hmm, it does?

What we really want to check here is that each pair[1] value is unique.
Again, I'd just add this to the error list. If someone really wants to do it, they can disable the checks. This should also happen if tune is used more than once.

Just set these and skip "doc" in the above tests. Its ugly but I'd prefer to have them set than unset.

Can you split this patch into two so we can get the bits that were below in (they look fine) whilst we work on the above.

Quoted 5 lines · On Tue, 1 May 2012 11:47:14 +0100 Richard Purdie…

To be honest I'm really tempted to split this tuneabi stuff out into a different class. I appreciate the OSVs need something but it probably doesn't make sense with half an implementation sitting in the core like this, particularly when the code is near impenetrable like the above. I'm left wondering what TUNEABI is too...

The big reason I'd advocate for having it in the base stuff is that otherwise, all the OSVs are going to do their own and do it incompatibly. The one thing worse than having one incomprehensible thing is having many incomprehensible things with subtly different semantics.

Quoted 2 lines

Quoted 2 lines

+ bb.warn("Got an unexpected '%s' in MULTILIBS." %
pairs[0])

Quoted 1 line

Shouldn't this get added to the errors list?

Good question! I was unable to find anything definite on the topic. My thought is, if everything has to be multilib:, there's no reason for the leading multilib: because it adds no information, so presumably there logically *could* be something else, but I haven't seen any and don't know about them.

Quoted 4 lines

Quoted 4 lines

meta/classes/sanity.bbclass +3 -0

+            else:
+                if pairs[1] == 'lib':
+                    tune_error_set.append("The multilib 'lib' was

specified, but that causes parse errors.")

Quoted 1 line

Hmm, it does?

Yeah, that was the thing I ran into when I tried a default config someone suggested; I thoughtfully corrected lib32 to lib, and libxcb exploded with two pages of Python stack traces and diagnostics. If that's not intentional, I can drop this check and we can treat it as a package bug.

Ah, I see what's happened here. This variable is used to append to BBCLASSEXTEND where there are other values like "nativesdk" and "native". Looking at the code, I think you should process MULTILIB_VARIANTS.

Another good sanity check might be to look at the BBCLASSEXTEND values and ensure they're in the list "native", "nativesdk", "multilib:xxx", "cross".

I think its a packaging bug. I'd be interested to see the error.