test: Preserve llvm profile path #32209

pull Prabhat1308 wants to merge 1 commits into bitcoin:master from Prabhat1308:pv/add_llvm_env changing 1 files +1 −5

Prabhat1308 commented at 6:48 AM on April 3, 2025: contributor

While generating profraw for fuzz tests using steps in PR 32206 , the profraw was not being built at the desired location and only one default.profraw was being created which was being overwritten for multiple fuzz targets. This PR fixes that.
DrahtBot commented at 6:48 AM on April 3, 2025: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32209.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK maflcko, mabu44

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label Tests on Apr 3, 2025
Prabhat1308 force-pushed on Apr 3, 2025
DrahtBot added the label CI failed on Apr 3, 2025
DrahtBot commented at 6:52 AM on April 3, 2025: contributor


🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/39897352535</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
- Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
- A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
- An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.

</details>
Prabhat1308 force-pushed on Apr 3, 2025
in test/fuzz/test_runner.py:36 in d6dc4073d7 outdated
```
  27 | @@ -28,6 +28,9 @@ def get_fuzz_env(*, target, source_dir):
  28 |          'ASAN_SYMBOLIZER_PATH': symbolizer,
  29 |          'MSAN_SYMBOLIZER_PATH': symbolizer,
  30 |      }
  31 | +    # Preserve LLVM_PROFILE_FILE if set
  32 | +    if 'LLVM_PROFILE_FILE' in os.environ:
  33 | +        fuzz_env['LLVM_PROFILE_FILE'] = os.environ['LLVM_PROFILE_FILE']
  34 |      if platform.system() == "Windows":
  35 |          # On Windows, `env` option must include valid `SystemRoot`.
  36 |          fuzz_env = {**fuzz_env, 'SystemRoot': os.environ.get('SystemRoot')}
```
maflcko commented at 6:59 AM on April 3, 2025:

I guess at some point it becomes easier to just create a full copy of os.environ and update it with the options set above, instead of touching this file for every env var that should be passed through.

Prabhat1308 commented at 10:48 AM on April 3, 2025:

This is bit of an unknown territory for me so I have implemented your suggestion to the best of my knowledge. Now any new env variable can be added inside fuzz_env. I don't need to add an if condition to update fuzz_env everytime with a variable now and think this should reduce the touches.
DrahtBot removed the label CI failed on Apr 3, 2025
Prabhat1308 force-pushed on Apr 3, 2025

Type	Reviewers
ACK	maflcko, mabu44

in test/fuzz/test_runner.py:23 in cf35f22a13 outdated

  19 | @@ -20,6 +20,7 @@
  20 |  def get_fuzz_env(*, target, source_dir):
  21 |      symbolizer = os.environ.get('LLVM_SYMBOLIZER_PATH', "/usr/bin/llvm-symbolizer")
  22 |      fuzz_env = {
  23 | +        **os.environ,

maflcko commented at 11:08 AM on April 3, 2025:

nit: It is the same, but you can probably use |.

    fuzz_env = os.environ | {

in test/fuzz/test_runner.py:34 in cf35f22a13 outdated

  29 | @@ -29,8 +30,8 @@ def get_fuzz_env(*, target, source_dir):
  30 |          'MSAN_SYMBOLIZER_PATH': symbolizer,
  31 |      }
  32 |      if platform.system() == "Windows":
  33 | -        # On Windows, `env` option must include valid `SystemRoot`.
  34 | -        fuzz_env = {**fuzz_env, 'SystemRoot': os.environ.get('SystemRoot')}
  35 | +        # On Windows, ensure SystemRoot is set
  36 | +        fuzz_env['SystemRoot'] = os.environ.get('SystemRoot')

maflcko commented at 11:08 AM on April 3, 2025:

that's a no-op?

Prabhat1308 commented at 3:26 PM on April 3, 2025:

I added this to make sure it is set since I was not completely sure if this is being set on windows (although it should be). Can't confirm this since I dont have windows

maflcko approved
maflcko commented at 11:08 AM on April 3, 2025: member

lgtm
Prabhat1308 force-pushed on Apr 3, 2025
maflcko commented at 3:29 PM on April 3, 2025: member

lgtm ACK 9dc3862f67a0f2a5feb55abc325d1ce760119160

preserve llvm profile env

Signed-off-by: Prabhat Verma <prabhatverma329@gmail.com>

c5a7ffd1e8

Prabhat1308 force-pushed on Apr 3, 2025
DrahtBot added the label CI failed on Apr 3, 2025
DrahtBot commented at 3:32 PM on April 3, 2025: contributor


🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/39929972727</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
- Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
- A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
- An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.

</details>
maflcko commented at 5:25 PM on April 3, 2025: member

lgtm ACK c5a7ffd1e8ccae12e034face4293bc8d5a6556b7
DrahtBot removed the label CI failed on Apr 3, 2025
mabu44 commented at 7:09 PM on April 3, 2025: none

ACK c5a7ffd1e8ccae12e034face4293bc8d5a6556b7 Tested on Linux with steps from #32206. LGTM for the Windows part.
maflcko commented at 7:21 AM on April 4, 2025: member

rfm, or does it need more review by other fuzz people?
fanquake commented at 7:40 AM on April 4, 2025: member

cc @dergoegge @marcofleon
fanquake merged this on Apr 4, 2025
fanquake closed this on Apr 4, 2025
Prabhat1308 deleted the branch on Apr 4, 2025

Contributors

Labels

Linked (view graph)

#32206 doc: Add fuzz based coverage report generation

test: Preserve llvm profile path #32209

Code Coverage & Benchmarks

Reviews