Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lit] Fix some issues from --per-test-coverage #65242

Merged
merged 1 commit into from
Sep 14, 2023

Commits on Sep 14, 2023

  1. [lit] Fix some issues from --per-test-coverage

    D154280 (landed in 64d1954 in July, 2023) implements
    `--per-test-coverage` (which can also be specified via
    `lit_config.per_test_coverage`).  However, it has a few issues, which
    the current patch addresses:
    
    1. D154280 implements `--per-test-coverage` only for the case that lit
       is configured to use an external shell.  The current patch extends
       the implementation to lit's internal shell.
    
    2. In the case that lit is configured to use an external shell,
       regardless of whether `--per-test-coverage` is actually specified,
       D154280 causes `%dbg(RUN: at line N)` to be expanded in RUN lines
       early and in a manner that is specific to sh-like shells.  As a
       result, later code in lit that expands it in a shell-specific
       manner is useless as there's nothing left to expand.  The current
       patch cleans up the implementation to avoid useless code.
    
    3. Because of issue 2, D154280 corrupts support for windows `cmd` as
       an external shell (effectively comments out all RUN lines with
       `:`).  The current patch happens to fix that particular corruption
       by addressing issue 2.  However, D122569 (landed in 1041a96 in
       April, 2022) had already broken support for windows `cmd` as an
       external shell (discards RUN lines when expanding `%dbg(RUN: at
       line N)`).  The current patch does not attempt to fix that bug.
       For further details, see the PR discussion of the current patch.
    
    The current patch addresses the above issues by implementing
    `--per-test-coverage` before selecting the shell (internal or
    external) and by leaving `%dbg(RUN: at line N)` unexpanded there.
    Thus, it is expanded later in a shell-specific manner, as before
    D154280.
    
    This patch introduces `buildPdbgCommand` into lit's implementation to
    encapsulate the process of building (or rebuilding in the case of the
    `--per-test-coverage` implementation) a full `%dbg(RUN: at line N)
    cmd` line and asserting that the result matches `kPdbgRegex`.  It also
    cleans up that and all other uses of `kPdbgRegex` to operate on the
    full line with `re.fullmatch` not `re.match`.  This change better
    reflects the intention in every case, but it is expected to be NFC
    because `kPdbgRegex` ends in `.*` and thus avoids the difference
    between `re.fullmatch` and `re.match`.  The only caveat is that `.*`
    does not match newlines, but RUN lines cannot contain newlines
    currently, so this caveat currently shouldn't matter in practice.
    
    The original `--per-test-coverage` implementation avoided accumulating
    `export LLVM_PROFILE_FILE={profile}` insertions across retries (due to
    `ALLOW_RETRIES`) by skipping the insertion if `%dbg(RUN: at line N)`
    was not present and thus had already been expanded.  However, the
    current patch makes sure the insertions also happen for commands
    without `%dbg(RUN: at line N)`, such as preamble commands or some
    commands from other lit test formats.  Thus, the current patch
    implements a different mechanism to avoid accumulating those
    insertions (see code comments).
    jdenny-ornl committed Sep 14, 2023
    Configuration menu
    Copy the full SHA
    c0fa7bc View commit details
    Browse the repository at this point in the history