This is an archive of the discontinued LLVM Phabricator instance.

Differential D140269

[BOLT] Check llvm-bolt binaries before running NFC checks
ClosedPublic

Authored by Amir on Dec 17 2022, 10:10 PM.

Details

Reviewers
gkistanova
Group Reviewers
Restricted Project
Commits
rZORGdc1dd89ac010: [BOLT] Check llvm-bolt binaries before running NFC checks
Summary

In NFC testing, if llvm-bolt binaries are bitwise identical,
don't run NFC checks.

Diff Detail

Event Timeline

Amir created this revision.Dec 17 2022, 10:10 PM
Herald added a reviewer: gkistanova. · View Herald TranscriptDec 17 2022, 10:10 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: treapster. · View Herald Transcript
Amir requested review of this revision.Dec 17 2022, 10:10 PM
Harbormaster completed remote builds in B203795: Diff 483803.Dec 17 2022, 10:10 PM
Amir updated this revision to Diff 483848.Dec 18 2022, 3:11 PM

Fix shell command

Harbormaster completed remote builds in B203832: Diff 483848.Dec 18 2022, 3:11 PM
Amir updated this revision to Diff 483849.Dec 18 2022, 3:12 PM

Remove unrelated changes

Harbormaster completed remote builds in B203833: Diff 483849.Dec 18 2022, 3:12 PM
Amir updated this revision to Diff 483850.Dec 18 2022, 3:13 PM

Remove util

Harbormaster completed remote builds in B203834: Diff 483850.Dec 18 2022, 3:13 PM
gkistanova added a comment.Dec 25 2022, 8:55 PM

Thanks, Amir.

The patch looks good. The only thing I'm not sure is the error handling at the check-bolt-different step. + one cosmetic nit pick.

zorg/buildbot/builders/BOLTBuilder.py
99

How about adding something like "and skip the following 2 steps otherwise" to the step description?

101

Could you elaborate on why you want flunkOnFailure=False here, please?

At a quick glance, the step fails only if (1) the touch command fails, or (2) depending on error handling mode of the buildbot account shell if remove file command fails. The both cases seems indicate severe enough issues to be aware of (i.e. to fail the build), even if they are not related to a commit in question.

Amir updated this revision to Diff 485458.Dec 27 2022, 9:58 PM

Update comment

Harbormaster completed remote builds in B205048: Diff 485458.Dec 27 2022, 9:58 PM
Amir marked 2 inline comments as done.Dec 27 2022, 10:01 PM

@gkistanova,

Thank you for a review!

zorg/buildbot/builders/BOLTBuilder.py
101

I agree with your reasoning that an issue with running cmp + touch is severe enough to warrant a test failure. I didn't think much of it, just copied the configuration from the following steps.

Amir marked an inline comment as done.Dec 27 2022, 10:01 PM
gkistanova accepted this revision.Dec 27 2022, 10:30 PM

LGTM.

This revision is now accepted and ready to land.Dec 27 2022, 10:30 PM
Amir updated this revision to Diff 485579.Dec 28 2022, 10:04 PM

Fix up paths

Harbormaster completed remote builds in B205132: Diff 485579.Dec 28 2022, 10:04 PM
Amir updated this revision to Diff 485580.Dec 28 2022, 10:05 PM

Remove unrelated changes

Harbormaster completed remote builds in B205133: Diff 485580.Dec 28 2022, 10:05 PM
Amir updated this revision to Diff 485663.Dec 29 2022, 7:35 PM

Fix up cmp command, ensure the checks/skips are working as intended:
http://157.230.108.44:8011/#/builders/55/builds/1339 modifies llvm-bolt, NFC checks enforced
http://157.230.108.44:8011/#/builders/55/builds/1340 doesn't touch llvm-bolt, NFC checks skipped

Harbormaster completed remote builds in B205196: Diff 485663.Dec 29 2022, 7:35 PM
Closed by commit rZORGdc1dd89ac010: [BOLT] Check llvm-bolt binaries before running NFC checks (authored by Amir). · Explain WhyDec 29 2022, 7:40 PM
This revision was automatically updated to reflect the committed changes.
Amir added a commit: rZORGdc1dd89ac010: [BOLT] Check llvm-bolt binaries before running NFC checks.

Revision Contents

PathSize
zorg/
buildbot/
builders/
12 lines

Diff 485664

zorg/buildbot/builders/BOLTBuilder.py

from buildbot.plugins import steps from buildbot.plugins import steps
from buildbot.steps.shell import ShellCommand from buildbot.steps.shell import ShellCommand
from zorg.buildbot.builders.UnifiedTreeBuilder import getLLVMBuildFactoryAndSourcecodeSteps, addCmakeSteps, addNinjaSteps
from zorg.buildbot.commands.LitTestCommand import LitTestCommand from zorg.buildbot.commands.LitTestCommand import LitTestCommand
from zorg.buildbot.commands.CmakeCommand import CmakeCommand from zorg.buildbot.commands.CmakeCommand import CmakeCommand
from zorg.buildbot.builders.UnifiedTreeBuilder import getLLVMBuildFactoryAndSourcecodeSteps, addCmakeSteps, addNinjaSteps from zorg.buildbot.conditions.FileConditions import FileExists
from zorg.buildbot.process.factory import LLVMBuildFactory from zorg.buildbot.process.factory import LLVMBuildFactory
def getBOLTCmakeBuildFactory( def getBOLTCmakeBuildFactory(
clean = False, clean = False,
bolttests = False, bolttests = False,
is_nfc = False, is_nfc = False,
targets = None, targets = None,
checks = None, checks = None,
▲ Show 20 Lines • Show All 73 Lines • ▼ Show 20 Lines if is_nfc:
ShellCommand( ShellCommand(
name='nfc-check-setup', name='nfc-check-setup',
command=[f"../{f.monorepo_dir}/bolt/utils/nfc-check-setup.py"], command=[f"../{f.monorepo_dir}/bolt/utils/nfc-check-setup.py"],
description=('Setup NFC testing'), description=('Setup NFC testing'),
warnOnFailure=True, warnOnFailure=True,
haltOnFailure=False, haltOnFailure=False,
flunkOnFailure=False, flunkOnFailure=False,
env=env), env=env),
ShellCommand(
name='check-bolt-different',
command='rm -f .llvm-bolt.diff; cmp -s bin/llvm-bolt.old bin/llvm-bolt.new || touch .llvm-bolt.diff',
description=('Check if llvm-bolt binaries are different and '
gkistanovaUnsubmitted
Done
ReplyInline Actions

How about adding something like "and skip the following 2 steps otherwise" to the step description?

gkistanova: How about adding something like "and skip the following 2 steps otherwise" to the step…
'skip the following nfc-check steps'),
haltOnFailure=False,
gkistanovaUnsubmitted
Done
ReplyInline Actions

Could you elaborate on why you want flunkOnFailure=False here, please?

At a quick glance, the step fails only if (1) the touch command fails, or (2) depending on error handling mode of the buildbot account shell if remove file command fails. The both cases seems indicate severe enough issues to be aware of (i.e. to fail the build), even if they are not related to a commit in question.

gkistanova: Could you elaborate on why you want `flunkOnFailure=False` here, please? At a quick glance…
AmirAuthorUnsubmitted
Done
ReplyInline Actions

I agree with your reasoning that an issue with running cmp + touch is severe enough to warrant a test failure. I didn't think much of it, just copied the configuration from the following steps.

Amir: I agree with your reasoning that an issue with running cmp + touch is severe enough to warrant…
env=env),
LitTestCommand( LitTestCommand(
name='nfc-check-bolt', name='nfc-check-bolt',
command=['bin/llvm-lit', '-sv', '-j4', command=['bin/llvm-lit', '-sv', '-j4',
# bolt-info will always mismatch in NFC mode # bolt-info will always mismatch in NFC mode
'--xfail=bolt-info.test', '--xfail=bolt-info.test',
# FIXME[aaupov]: https://github.com/llvm/llvm-project/issues/59008 # FIXME[aaupov]: https://github.com/llvm/llvm-project/issues/59008
'--filter-out=X86/bb-with-two-tail-calls.s', '--filter-out=X86/bb-with-two-tail-calls.s',
'tools/bolt/test'], 'tools/bolt/test'],
description=["running", "NFC", "check-bolt"], description=["running", "NFC", "check-bolt"],
descriptionDone=["NFC", "check-bolt", "completed"], descriptionDone=["NFC", "check-bolt", "completed"],
warnOnFailure=True, warnOnFailure=True,
haltOnFailure=False, haltOnFailure=False,
flunkOnFailure=False, flunkOnFailure=False,
doStepIf=FileExists('build/.llvm-bolt.diff'),
env=env), env=env),
LitTestCommand( LitTestCommand(
name='nfc-check-large-bolt', name='nfc-check-large-bolt',
command=['bin/llvm-lit', '-sv', '-j2', command=['bin/llvm-lit', '-sv', '-j2',
'tools/bolttests'], 'tools/bolttests'],
description=["running", "NFC", "check-large-bolt"], description=["running", "NFC", "check-large-bolt"],
descriptionDone=["NFC", "check-large-bolt", "completed"], descriptionDone=["NFC", "check-large-bolt", "completed"],
warnOnFailure=True, warnOnFailure=True,
haltOnFailure=False, haltOnFailure=False,
flunkOnFailure=False, flunkOnFailure=False,
doStepIf=FileExists('build/.llvm-bolt.diff'),
env=env), env=env),
]) ])
return f return f