Commits on Nov 16, 2021

1. ISLE: guard against stale generated source in default build config. …

   Currently, the `build.rs` script that generates Rust source from the
   ISLE DSL will only do this generation if the `rebuild-isle` Cargo
   feature is specified. By default, it is not. This is based on the
   principle that we (the build script) do not modify the source tree as
   managed by git; git-managed files are strictly a human-managed and
   human-edited resource. By adding the opt-in Cargo feature, a developer
   is requesting the build script to perform an explicit action. (In my
   understanding at least, this principle comes from the general philosophy
   of hermetic builds: the output should be a pure function of the input,
   and part of this is that the input is read-only. If we modify the source
   tree, then all bets are off.)
   
   Unfortunately, requiring the opt-in feature also creates a footgun that
   is easy to hit: if a developer modifies the ISLE DSL source, but forgets
   to specify the Cargo feature, then the compiler will silently be built
   successfully with stale source, and will silently exclude any changes
   that were made.
   
   The generated source is checked into git for a good reason: we want DSL
   compiler to not affect build times for the overwhelmingly common case
   that Cranelift is used as a dependency but the backends are not being
   actively developed. (This overhead comes mainly from building `islec`
   itself.)
   
   So, what to do? This PR implements a middle ground first described in
   [this conversation](bytecodealliance#3506 (comment)), in which we:
   
   - Generate a hash (SHA-512) of the ISLE DSL source and produce a
     "manifest" of ISLE inputs alongside the generated source; and
   - Always read the ISLE DSL source, and see if the manifest is still
     valid, on builds that do not have the opt-in "rebuild" feature.
   
   This allows us to know whether the ISLE compiler output would have been
   the same (modulo changes to the DSL compiler itself, which are
   out-of-scope here), without actually building the ISLE compiler and
   running it.
   
   If the compiler-backend developer modifies an ISLE source file and then
   tries to build `cranelift-codegen` without adding the `rebuild-isle`
   Cargo feature, they get the following output:
   
   ```text
     Error: the ISLE source files that resulted in the generated Rust source
   
           * src/isa/x64/lower/isle/generated_code.rs
   
     have changed but the generated source was not rebuilt! These ISLE source
     files are:
   
            * src/clif.isle
            * src/prelude.isle
            * src/isa/x64/inst.isle
            * src/isa/x64/lower.isle
   
     Please add `--features rebuild-isle` to your `cargo build` command
     if you wish to rebuild the generated source, then include these changes
     in any git commits you make that include the changes to the ISLE.
   
     For example:
   
       $ cargo build -p cranelift-codegen --features rebuild-isle
   
     (This build script cannot do this for you by default because we cannot
     modify checked-into-git source without your explicit opt-in.)
   ```
   
   which will tell them exactly what they need to do to fix the problem!
   
   Note that I am leaving the "Rebuild ISLE" CI job alone for now, because
   otherwise, we are trusting whomever submits a PR to generate the correct
   generated source. In other words, the manifest is a communication from
   the checked-in tree to the developer, but we still need to verify that
   the checked-in generated Rust source and the manifest are correct with
   respect to the checked-in ISLE source.

   

   cfallin committed Nov 16, 2021
   Configuration menu

   • View commit details
   • Copy full SHA for a2b9664
   • Browse repository at this point

   Copy the full SHA

   a2b9664 View commit details

   Browse the repository at this point in the history

2. Two CI fixes: Windows line-endings in manifest file, and "meta determ… …

   …inistic check".
   
   - The Windows line-ending canonicalization was incomplete: we need to
     canonicalize the manifest text itself too!
   
   - The "meta deterministic check" runs the cranelift-codegen build script
     N times outside of the source tree, examining what it produces to
     ensure the output is always the same (is detministic). This works fine
     when everything comes from the internal DSL, but when reading ISLE,
     this breaks because we no longer have the ISLE source paths.
   
     The initial ISLE integration did not hit this because without the
     `rebuild-isle` feature, it simply did nothing in the build script;
     now, with the manifest check, we hit the issue.
   
     The fix for now is just to turn off all ISLE-specific behavior in the
     build script by setting a special-purpose Cargo feature in the
     specific CI job. Eventually IMHO we should remove or find a better way
     to do this check.

   

   cfallin committed Nov 16, 2021
   Configuration menu

   • View commit details
   • Copy full SHA for 6a4716f
   • Browse repository at this point

   Copy the full SHA

   6a4716f View commit details

   Browse the repository at this point in the history

3. ISLE build-script check logic: Canonicalize All The Things! …

   writeln!() to a string on Windows to generate the expected manifest
   needs newline-character canonicalization, too.

   

   cfallin committed Nov 16, 2021
   Configuration menu

   • View commit details
   • Copy full SHA for bc56ab7
   • Browse repository at this point

   Copy the full SHA

   bc56ab7 View commit details

   Browse the repository at this point in the history

4. Let's try canonicalizing file paths in the manifest too...

   

   cfallin committed Nov 16, 2021
   Configuration menu

   • View commit details
   • Copy full SHA for 1697e32
   • Browse repository at this point

   Copy the full SHA

   1697e32 View commit details

   Browse the repository at this point in the history