This is an archive of the discontinued LLVM Phabricator instance.

[sanitizers] Controlling the build width and targets for PowerPC sanitizers
ClosedPublic

Authored by kamaub on Oct 12 2022, 12:27 PM.

Details

Summary

The cmake sanitizer buildbots use the nproc value for the build
width by default, this means the PowerPC sanitizer buildbots were
building and testing at the max parallelism their host machines could
handle.
This patch adds a build and test width specification for the PPC
sanitizers and sets the build targets to only build for PowerPC
with the intension of reducing the load the sanitizers have on their
host while improving build times. It fixes up cmake argument passing
to facilitate the change.

Diff Detail

Event Timeline

kamaub created this revision.Oct 12 2022, 12:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2022, 12:27 PM
kamaub requested review of this revision.Oct 12 2022, 12:27 PM
vitalybuka added inline comments.Oct 12 2022, 12:42 PM
buildbot/osuosl/master/config/builders.py
1753

-DLLVM_CCACHE_BUILD=ON is not needed as it's enabled by buildbot_cmake.sh:31

also it should not go into CMAKE_COMMON_OPTIONS as only the first stage can benefit of that

1754–1755

why the first one 80 and this one 256?

zorg/buildbot/builders/sanitizers/buildbot_cmake.sh
23

maybe better to detect arch(or even bot name) and make adjustments in buildbot_cmake.sh

for UnifiedTreeBuilder it make sense to keep them in builders.py, but SanitizerBuilder most configuration happening sh files already, and it easier to see when I edit the file.

also sh applies immediately and does not need to wait staging->buildbot release/reboot

Thank you for the review, I am testing these changes at the moment, will post the update afterwards.

buildbot/osuosl/master/config/builders.py
1753

Could point, I was originally just adapting the code since passing the argument the previous way just get's it ignored, but I will remove this in the next update.

1753

Good point*

1754–1755

These values are based on the host machine and takes into consideration the load of the other buildbots running on them; our Big Endian host has a smaller capacity than our Little Endian host. We're okay with make and lit starting these many jobs but not with using the full nproc.

zorg/buildbot/builders/sanitizers/buildbot_cmake.sh
23

No worries, thank you for maintaining the sanitizers, I will move the cmake arguments to the target specific section of buildbot_cmake.sh and control for endianness.

kamaub updated this revision to Diff 468511.Oct 18 2022, 6:07 AM

Addressing review comments

amyk added inline comments.Nov 2 2022, 8:25 PM
zorg/buildbot/builders/sanitizers/buildbot_cmake.sh
103

It looks like there is an extra ' at the end of the -v value. Is this intended?

106

It looks like there is an extra ' at the end of the -v value. Is this intended?

lei added inline comments.Nov 4 2022, 5:52 AM
zorg/buildbot/builders/sanitizers/buildbot_cmake.sh
100

I don't think we need ' around PowerPC

kamaub updated this revision to Diff 473750.Nov 7 2022, 11:17 AM

Addressing review comments

kamaub marked 3 inline comments as done.Nov 7 2022, 11:18 AM

Thank you for the review, I have removed the necessary quotations.

amyk accepted this revision.Nov 10 2022, 7:29 AM

Thanks for addressing my comments. I think unless if anyone else has any concerns, this LGTM.

This revision is now accepted and ready to land.Nov 10 2022, 7:29 AM