ci: Add missing -O2 to valgrind tasks

maflcko commented at 1:48 pm on July 12, 2023: member

Currently the tasks have nothing (-O0) set, which makes them slow.

Fix this by falling back to the default (-O2).

ci: Add missing -O2 to valgrind tasks fa4ccf1511

DrahtBot commented at 1:48 pm on July 12, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	dergoegge, recursive-rat4

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27992 (ci: build Valgrind (3.21) from source by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Tests on Jul 12, 2023

dergoegge commented at 2:44 pm on July 12, 2023: member

which makes them slow

How much faster are they with -O2?

maflcko commented at 3:43 pm on July 12, 2023: member

How much faster are they with -O2?

Yes.

(Locally the functional tests go from 77837 s (accumulated) to 19769 s (accumulated))

dergoegge commented at 3:45 pm on July 12, 2023: member

(Locally the functional tests go from (hasn’t finished) to 19769s (accumulated))

Any results for the fuzz task? Just to compare it to the speed up observed in #27992

maflcko commented at 4:51 pm on July 12, 2023: member

Yes.

From:

 0Run asmap_direct with args ['valgrind', '--quiet', '--error-exitcode=1', '/root/bitcoin/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/root/bitcoin/ci/scratch/qa-assets/fuzz_seed_corpus/asmap_direct')]INFO: Running with entropic power schedule (0xFF, 100).
 1INFO: Seed: 98738393
 2INFO: Loaded 1 modules   (248427 inline 8-bit counters): 248427 [0x2781858, 0x27be2c3), 
 3INFO: Loaded 1 PC tables (248427 PCs): 248427 [0x27be2c8,0x2b88978), 
 4INFO:      645 files found in /root/bitcoin/ci/scratch/qa-assets/fuzz_seed_corpus/asmap_direct
 5INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1048576 bytes
 6INFO: seed corpus: files: 645 min: 1b max: 1048576b total: 15304310b rss: 169Mb
 7[#512](/bitcoin-bitcoin/512/)	pulse  cov: 329 ft: 1557 corp: 243/22Kb exec/s: 21 rss: 188Mb
 8[#646](/bitcoin-bitcoin/646/)	INITED cov: 329 ft: 1576 corp: 257/39Kb exec/s: 5 rss: 219Mb
 9[#646](/bitcoin-bitcoin/646/)	DONE   cov: 329 ft: 1576 corp: 257/39Kb lim: 2177 exec/s: 5 rss: 219Mb
10Done 646 runs in 128 second(s)

To:

 0Run asmap_direct with args ['valgrind', '--quiet', '--error-exitcode=1', '/root/bitcoin/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/root/bitcoin/ci/scratch/qa-assets/fuzz_seed_corpus/asmap_direct')]INFO: Running with entropic power schedule (0xFF, 100).
 1INFO: Seed: 1921218028
 2INFO: Loaded 1 modules   (362668 inline 8-bit counters): 362668 [0x1c58000, 0x1cb08ac), 
 3INFO: Loaded 1 PC tables (362668 PCs): 362668 [0x1cb08b0,0x2239370), 
 4INFO:      645 files found in /root/bitcoin/ci/scratch/qa-assets/fuzz_seed_corpus/asmap_direct
 5INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1048576 bytes
 6INFO: seed corpus: files: 645 min: 1b max: 1048576b total: 15304310b rss: 196Mb
 7[#512](/bitcoin-bitcoin/512/)	pulse  cov: 238 ft: 1316 corp: 367/51Kb exec/s: 128 rss: 216Mb
 8[#646](/bitcoin-bitcoin/646/)	INITED cov: 238 ft: 1372 corp: 395/91Kb exec/s: 35 rss: 248Mb
 9[#646](/bitcoin-bitcoin/646/)	DONE   cov: 238 ft: 1372 corp: 395/91Kb lim: 8193 exec/s: 35 rss: 248Mb
10Done 646 runs in 18 second(s)

recursive-rat4 commented at 6:55 pm on July 12, 2023: none

Why not CXXFLAGS='$CXXFLAGS -gdwarf-4'?

maflcko commented at 6:06 am on July 13, 2023: member

Why not CXXFLAGS=’$CXXFLAGS -gdwarf-4'?

Feel free to test it and make an alternative pull, but I am pretty sure it will not work. Also, I don’t see any upside and it is more verbose.

dergoegge approved

dergoegge commented at 11:57 am on July 13, 2023: member

utACK fa4ccf15118a5e2dcd2a98283b9e6c7e1955a7f1

recursive-rat4 commented at 6:27 pm on July 13, 2023: none

utACK fa4ccf15118a5e2dcd2a98283b9e6c7e1955a7f1

CXXFLAGS was set in #24735 without intention to disable optimizations.

maflcko commented at 6:34 pm on July 13, 2023: member

rfm or is something missing here?

fanquake commented at 6:42 pm on July 13, 2023: member

Use of -O2 and above is not recommended as Memcheck occasionally reports uninitialised-value errors which don’t really exist.

I guess this doesn’t matter for us/is no-longer the case in Memcheck? Some mailing list discussion suggests so.

I think there’s another issue here (for a different PR) where we should be setting some optimisation level in our build flags, because setting additional flags should not this problem.

maflcko commented at 6:46 pm on July 13, 2023: member

I guess this doesn’t matter for us/is no-longer the case in Memcheck? Some mailing list discussion suggests so.

Only for gcc, see #27741

I think there’s another issue here (for a different PR) where we should be setting some optimisation level in our build flags, because setting additional flags should not this problem.

Right. Probably not worth to spend time here, with the switch to cmake. But that makes me wonder how cmake handles this.

fanquake commented at 9:54 am on July 14, 2023: member

Right. Probably not worth to spend time here,

I’ll follow up with this, because it certainly shouldn’t be broken, and if it is, it’s not specific to valgrind etc.

fanquake merged this on Jul 14, 2023

fanquake closed this on Jul 14, 2023

maflcko deleted the branch on Jul 14, 2023

bitcoin locked this on Jul 13, 2024

ci: Add missing -O2 to valgrind tasks #28071

Reviews

Conflicts