Skip to content

Conversation

@Johanmyst
Copy link
Contributor

As per @yuleisui's request in this pull request (1697), this pull request contains the various fixes, improvements, and other changes unrelated to the build system. Note that this pull request contains one additional commit [c1306f4] not in the original pull request [1697], which adds some additional allocator definitions to extapi.c and ensures existing ones are consistent.

📝 Overview

This PR consolidates six orthogonal areas of work into one release:

  • CI/Workflow: harden coverage removal flags in GitHub Actions
  • CMake Formatting: add cmake-format config for consistent styling & formatting
  • API Additions: add missing has…() checks alongside existing getters
  • WPA Backends: unify public interface for polymorphic use
  • SVFG Builder: fix read/write order for optimised graphs
  • Allocators: add & fix some allocator definitions

For full details on each change, see the linked commits below.


🚀 Highlights

Area Summary Commit
CI/Workflow Uniform --ignore-errors unused on all lcov --remove 96c021b
CMake Formatting Add cmake-format spec for automatic styling 0bc1472
API Existence New has…() functions for previously unchecked getters ce288cf
WPA Polymorphism Publicise ctor/read/dump APIs in VersionedFlowSensitive 93446f6
SVFG Builder Write unoptimised graph before optimise; explicit flag def7c04
Allocators Add missing allocators; fix restricted keyword; misc fixes 583bff0

⚙️ Detailed Changes

1️⃣ CI/Workflow Fix ([96c021b])

  • Always pass --ignore-errors unused to all lcov --remove … calls
  • Prevent workflow failures when no coverage data exists under /usr/* or other paths
  • Ensured error suppression is applied consistently

2️⃣ Automated CMake Formatting ([0bc1472])

  • Add .cmake-format.py so cmake-format can lint & format all CMake files
  • Ensures consistent styling across CMakeLists.txt, *.cmake, etc.

3️⃣ Existence-Check APIs ([ce288cf])

  • Introduce hasLLVMValue(), hasLHSTopLevelPtr(), etc., for getters that assert on missing entries
  • Avoids breaking existing API: getters still assert, but you can pre-check
  • Getters that return nullptr on missing entries are unmodified

4️⃣ WPA Backend Polymorphism ([93446f6])

  • Mark VersionedFlowSensitive’s printStat(), dumpTopLevelPtsTo(), solveAndWritePtsToFile(), readPtsFromFile(), etc., as public
  • Enables code like:
    SVF::BVDataPTAImpl *pta = nullptr;
    switch(backend) {
      case Andersen_BASE:   pta = new Andersen(...);            break;
      case VFS_WPA:         pta = new VersionedFlowSensitive(...); break;
      default:              /**/;                              break;
    }
    

5️⃣ Optimised SVFG Read/Write Fix ([def7c04])

  • Add builder argument to build optimised SVFG without command-line option (to support using SVF as a library)
  • Write unoptimised SVFG before optimisation
  • Read unoptimised SVFG, then apply optimisation
  • Resolves crashes when reading a post-optimised file

6️⃣ Allocators ([c1306f4])

  • Add missing allocator definitions for C++ allocators (e.g., new with alignment operators; new with different internal definitions of size_t, etc.)
  • Add miscellaneous missing allocators (e.g., pvalloc(), strndup(), etc.)
  • Fix restrict keyword usage to suppress compiler/IDE warnings (use __restrict instead (supported by C & C++))
  • Made re-definition of NULL conditional (already defined in included header) to suppress compiler/IDE warnings

Johanmyst added 6 commits May 8, 2025 11:43
The current GitHub Workflow coverage test removes coverage information from certain files (i.e., with `lcov --remove coverage.info '/usr/*' --output-file coverage.info`). If the given directory (e.g., `/usr/*`) contains no files with coverage information, trying to remove this coverage information throws an error. The current workflow only adds `--ignore-errors unused` for the files with coverage information in `${{github.workspace}}/svf/include/FastCluster/*`, but not for the other directories. As this information should be removed anyway (also for the other directories), not having coverage information to begin with shouldn't be seen as an error. This commit applies the `--ignore-errors unused` flag consistently to the other `lcov --remove ...` invocations.
While there are formatting description files for C/C++ (i.e., `.clang-format`), no equivalent formatting specification exists for CMake (to automatically format CMake files (e.g., `CMakeLists.txt`) consistently with `cmake-format`). This commit adds such a format description to ensure all CMake files are formatted consistently.
Most `get...()` functions have `has...()` equivalents (e.g., `getDefSVFGNode()` and `hasDefSVFGNode()`). However, certain getters (e.g., `getLLVMValue()`, `getLHSTopLevelPtr()` had no such existence-checking counterparts. This commit adds a number of these checking functions in cases where they were missing.

Note that these are for cases where the getters fail an assertion when no valid value exists. Cases where the getter simply returns a null-pointer on failure to find any corresponding values (e.g., `ICFG::getFunEntryBlock()`, `ICFG::getFunExitBlock()`, etc.), so a null-check can be used to catch cases where the getter fails to get any corresponding object, and thus don't need a corresponding existence checker. To avoid changing the current behaviour of the assertions and getters, I added additional existence-checking functions. This does mean the container will be searched twice (once in the existence-check and then again in the getter itself) and thus wastes performance. To avoid this, all current getters could be modified so that all getters return null-pointers on failures, but I didn't want to make such an intrusive change in this commit.
…istent with other WPA implementations)

Currently, most WPA implementations (e.g., `AndersenWaveDiff`) expose read-only functions (e.g., `printStat()`, `dumpTopLevelPtsTo()`, etc.) publically, as well as "constructors" like `solveAndWritePtsToFile()`, `readPtsFromFile()`, etc. However, `VersionedFlowSensitive` is the only one to mark these functions as `private`/`protected`. This makes it annoying to use polymorphism over the underlying PTA base class (i.e., `BVDataPTAImpl`) and the overridden virtual functions. This commit fixes that by marking these functions as public in the `VersionedFlowSensitive` WPA class.

This change allows me to use polymorphism to conveniently try different WPA backends like this:

```
SVF::BVDataPTAImpl *pta = nullptr;

switch(getChosenBackend()) {
    case Andersen_BASE:
        pta = new Andersen(...);
        break;
    case VFS_WPA:
        pta = new VersionedFlowSensitive(...);
        break;
    default:
        break;
}
```
…existing results

The current `SVFGBuilder` optimises the SVFG based on the command-line argument. However, when using SVF as a library, this is inconvenient. This commit changes the builder to use an additional flag to control optimisation behaviour.

Moreover, this commit fixes the reading/writing issues for optimised SVFGs. Currently, the writing happens post-optimisation, which causes the reading step to fail due to missing/unexpected nodes. This commit also ensures the *unoptimised SVFG* is written to a file *prior to optimising it*, and reads an unoptimised SVFG from a file and then optimises it. This fixes these crashing issues.
…ress errant warnings; added definitions of C++ aligned allocators (i.e., `new(align)`); defined all plain/no-throw/aligned/aligned-no-throw variants of C++ allocators for `size_t` defined as `unsigned int` and `unsigned long`; and defined `pvalloc()` and `strndup()` as allocators
@codecov
Copy link

codecov bot commented May 8, 2025

Codecov Report

Attention: Patch coverage is 55.55556% with 12 lines in your changes missing coverage. Please review.

Project coverage is 63.68%. Comparing base (35ad982) to head (97bb59c).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
svf/lib/Graphs/SVFGOPT.cpp 0.00% 12 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1702      +/-   ##
==========================================
- Coverage   63.70%   63.68%   -0.03%     
==========================================
  Files         244      244              
  Lines       25962    25975      +13     
  Branches     4575     4574       -1     
==========================================
+ Hits        16539    16541       +2     
- Misses       9423     9434      +11     
Files with missing lines Coverage Δ
svf-llvm/include/SVF-LLVM/LLVMModule.h 97.72% <ø> (ø)
svf/include/Graphs/SVFGOPT.h 0.00% <ø> (ø)
svf/include/Graphs/VFG.h 90.72% <ø> (ø)
svf/include/MSSA/SVFGBuilder.h 100.00% <100.00%> (ø)
svf/include/WPA/VersionedFlowSensitive.h 100.00% <ø> (ø)
svf/lib/MSSA/SVFGBuilder.cpp 89.18% <100.00%> (+3.07%) ⬆️
svf/lib/Graphs/SVFGOPT.cpp 0.00% <0.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yuleisui
Copy link
Collaborator

yuleisui commented May 8, 2025

Thanks for splitting it.

Please address my comments in this patch: def7c04

I shall merge.

@Johanmyst
Copy link
Contributor Author

No problem, you can find the requested changes in 97bb59c.

The reason I'd implemented the Options::... behaviour this way is to be consistent with the Options::SVFGWithIndirectCall() behaviour. To remain consistent, I've also updated how that flag is handled according to your requested changes. Let me know if it's alright like this!

@yuleisui yuleisui merged commit 0f8a828 into SVF-tools:master May 8, 2025
5 checks passed
@Johanmyst Johanmyst deleted the various-fixes branch May 8, 2025 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants