test: Preserve llvm profile path

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. Debug: https://github.com/bitcoin/bitcoin/runs/39897352535

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.

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

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 |.

0    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. Debug: https://github.com/bitcoin/bitcoin/runs/39929972727

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.

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

test: Preserve llvm profile path #32209

Code Coverage & Benchmarks

Reviews