[PATCH] D130954: [DeadArgElim] Clear state before returning (NFC).

[Michele Scandale via Phabricator via llvm-commits]

michele.scandale added a comment.

In D130954#3698745 <https://reviews.llvm.org/D130954#3698745>, @nikic wrote:

> Are these three patches the only ones needed to support this? The main argument in favor of allowing this I see is that function passes *can* be reused (after all, we call them for each function), so not allowing reuse of module passes is "inconsistent".

**Short answer:** it depends on what "supporting this" mean.

Here's the description of how I discovered these issues after I encountered the problem with the `ModuleInlinerWrapperPass`.

I used a modified version of `opt` to do the following (with some additional hacks (*)):

- serialize to bitcode (preserving use-list order) the input module before any pass is being run
- allocate a new context
- materialize the module from bitcode in the new context
- run the pass manager (excluding output writing pass) on the newly materialized module
- destroy module and context
- continue with regular flow of opt and run the original module with the same pass manager

(*) To reduce the number of false failures, for the first run of the pass manager on the cloned module I have used a custom error handler in the context to drop errors and preventing `exit(1)` from the default handler, I have swapped the underlying file descriptors used by `llvm::outs()` and `llvm::errs()` with new ones pointing to files to prevent polluting the output stream with messages from the first run that might cause `FileCheck` failures. Moreover after the first run I reset the statistics, the debug counters (I locally added a `resetCounters` function to `DebugCounter`), all the analysis managers, and the data structures in `DebugInfoBeforePass`.

With the patches D130952 <https://reviews.llvm.org/D130952>, D130954 <https://reviews.llvm.org/D130954>, and D130955 <https://reviews.llvm.org/D130955> there are still some failures:

1. some tests in `Analysis/BasicAA` and `Analysis/CFLAliasAnalysis/Steensgaard` -- `AAEvaluator` counters -- these persist across invocation of `run` and the report is printed when the pass object is destroyed
2. `Transforms/FunctionImport/funcimport_cutoff.ll` -- `FunctionImport` has a global counter (a function scope static variable `ImportCount`) that is used for debug purposes (cutoff option used in some tests to control how many functions should be imported)
3. `Transforms/Inline/cgscc-inline-replay.ll` fails in one case due to the fact an error is raised due to failure in finding the replay file, but the custom diagnostic handler does not cause the execution to stop, and a nullptr is being dereferenced after that
4. `Transforms/Inline/inline_stats.ll` fails because the inlining statistic object is not reset
5. `Transforms/WholeProgramDevirt/devirt-single-impl2.ll` because when `DevirtModule` fails to read the summary from the file specified via the command line option `-whoelprogramdevirt-read-summary` it exits printing an error message, but during the first run of the PM `llvm::errs()` has been redirected, hence the FileCheck failure
6. some of the `CodeGen/BPF` and `CodeGen/BPF/CORE` tests fail because in the `BPFAbstractMemoryAccess` function pass there is a static map referencing `GlobalVariable` objects -- which is never cleared, hence when once the module containing such globals is gone there are dangling references that might be accessed in subsequent runs
7. `Other/ChangePrinters/print-changed-diff.ll`, `Other/change-printer.ll` -- the state of `InitialIR` of `ChangeReporter` cannot be reset hence we miss some expected lines from the second run
8. `Other/print-on-crash.ll` -- crash during first run where `llvm::errs()` and `llvm::outs()` have been redirected
9. `Other/opt-bisect-new-pass-manager.ll` -- cannot reset the bisect limit

For (6) we have `BPFAbstractMemoryAccess` as a function pass that can insert global variables in a module (and caches them in some static global map) -- I believe function passes are allowed to insert global variables, so I wonder if `BPFAbstractMemoryAccess` should be modified to be a module pass -- which might allow the removal of the static global map entirely.

The other failures I believe are all related to debug features.

The patches D130952 <https://reviews.llvm.org/D130952>, D130954 <https://reviews.llvm.org/D130954>, and D130955 <https://reviews.llvm.org/D130955> address problems that could occur in a non-debug scenario.
I don't know if it is acceptable to have exceptions for the debug related features, or if the issue I described should be addressed somehow.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130954/new/

https://reviews.llvm.org/D130954