[buildbot] Add check-fuzzer to Asan buildbot on Windows.

[zturner]

Can you delete and re-create, and add llvm-commits to the subscribers? (It doesn't work if you add it after the fact, it has to be done the first time you create the review)

[zturner]

Hmm, actually it looks like llvm-commits is already added, I'm not sure why it's not sending out email

[mpividori]

Zturner, but I added llvm-commit when I created the diff. It looks like it doesnt work...

[zturner]

Ahh it just sent out the email, I guess it was delayed.

[zturner]

zorg/buildbot/builders/SanitizerBuilderWindows.py

133

Should this be -DLLVM_USE_SANITIZER_COVERAGE=YES? (The R is missing in Sanitizer)

[mpividori]

zorg/buildbot/builders/SanitizerBuilderWindows.py

133

@zturner the flag is LLVM_USE_SANITIZE_COVERAGE (yes it is confusing...).

[mpividori]

+ Split build and testing into 2 steps so it is easier to understand when it fails.
+ Add asan dll path to Path, so it is found when executing the tests.
+ Add bin dir to Path, so tools like FileCheck, not and sancov are found when executing the tests.

[zturner]

rnk, does this look correct to you? I dont' see anything obviously wrong, but you are more familiar with this bot than I am.

zorg/buildbot/builders/SanitizerBuilderWindows.py

128

The backslashes here need to be escaped or they will be treated as escape characters. Or use path.join so you don't have to deal with slashes at all.

[mpividori]

Use os.path.join instead of backslashes.

[rnk]

zorg/buildbot/builders/SanitizerBuilderWindows.py

128

This won't work, this code will execute on the master, not the build slave, so the CWD will be wrong, and we won't be able to glob. I think it might be possible to construct a relative path to clang-cl, but I'm not sure how to add the absolute runtime library path to PATH.

[mpividori]

@rnk I am not sure I understand what you mean. All the steps will be executed in the same machine, won't they? So, after building clang and compiler-rt with MSVC, we have the binaries and libraries in build_dir\Release . I am adding them to PATH.

[mpividori]

@rnk Ah you mean: abspath . Ok I got it.

[zturner]

Ahh yes, classic master/slave problems :(

I had this problem once before, I think in the LLDB builder, and Galina showed me a way to get the current directory on the master, but I'll have to do some digging to find it.

[zturner]

Ok here it is.

# get full path to build directory
f.addStep(SetProperty(name="get_builddir",
                      command=["pwd"],
                      property="builddir",
                      description="set build dir",
                      workdir=build_dir))

# Use the build directory via the %(builddir) syntax.
cmake_cmd = [
    "cmake", "-G", "Ninja", "../llvm",
    "-DCMAKE_BUILD_TYPE=" + config,
    "-DPYTHON_HOME=" + python_source_dir,
    "-DCMAKE_INSTALL_PREFIX=../install",
    "-DLLDB_TEST_COMPILER=\"%(builddir)s/bin/clang.exe\""
    ]

[mpividori]

Ok. Now it should work fine. Would you agree?

[mpividori]

Done. Let me know if you agree. Do I need a special permission to push the commit?

[zturner]

zorg/buildbot/builders/SanitizerBuilderWindows.py

149–150

Does this work? I know Property('slave_env') will return the slave environment as a dictionary, but is this just a local dictionary where changes made to it don't get written back to the slave? That's what I expect.

If so you might need to do something like

newPath = ";".join([bin_path, dll_path, Property('slave_env')['Path']])
f.addStep(SetProperty(name="set toolchain path",
                          command=["set", "PATH", "=", newPath],
                          description="set toolchain path",
                          workdir=build_dir))

(I'm honestly not 100% sure though, so confirm that I'm right if you can)

[zturner]

Actually wait, each step runs with env=Property('slave_env'). So even if you set the path beforehand, this will override your changes because it will already have PATH in this env. So you need to save off the value of Property(slave_env), fix up the PATH environment variable, and then use that as the value of the env keyword argument in subsequent calls.

[mpividori]

@zturner I am modifying the Property('slave_env') dictionary. I update the value mapped to the key: Path , to add the new directories.

[rnk]

zorg/buildbot/builders/SanitizerBuilderWindows.py

134

This might fail if many version of clang have been built and the lib directory contains more than one version. I suspect the bot already has more than one version directory.

141

I doubt this will work. A "Property" is not a string or dictionary. The master never sees the property values until the build actually executes. It has to generate the list of steps to execute up front, before actually running any of them. When you say "addStep", that adds a step to be run in the future on another machine. The steps don't run synchronously inside this python code.

As an alternative, we don't need to set PATH if we link statically, right? I'd recommend we do that. Set "-DLLVM_USE_CRT_"+config.upper()"=MT" in the cmake command to do that.

[zturner]

In D29677#670869, @mpividori wrote:

@zturner I am modifying the Property('slave_env') dictionary. I update the value mapped to the key: Path , to add the new directories.

It might work if you save the value of Property('slave_env') into a local variable first, then modify the values in that local variable, then reference the local variable in all subsequent lines that say env = Property('slave_env'). Otherwise I don't know if it will just ask the slave for its value new every time. (This is all very magic black-box type stuff).

[zturner]

Or maybe I'm totally wrong and what Reid said is correct, that the master never sees the value of the properties :) I'm obviously not going to be putting buildbot on my resume :)

[mpividori]

zorg/buildbot/builders/SanitizerBuilderWindows.py

134

@rnk yes I considered that. But we are removing the build directory before starting, with the step: RemoveDirectory(name='clean '+build_dir

[mpividori]

Let mw know if you agree with this diff. I use Interpolate() and I update PATH in the slave_env.

zorg/buildbot/builders/SanitizerBuilderWindows.py

141

@rnk Thanks. We also need to set PATH because we need to add Filecheck, not, sancov to PATH.

[rnk]

zorg/buildbot/builders/SanitizerBuilderWindows.py

134

No, that step is conditional on cleanBuildIfRequested, which is typically false. The builder is intended to be fast and incremental.

[rnk]

zorg/buildbot/builders/SanitizerBuilderWindows.py

141

True, but we don't have to compute the clang version to compute the path the bin directory. Static linking would still eliminate the need for this "dir /b" step.

143

Why add config? A ninja build will put the binaries in "%(builddir)s/bin", right?

148

This will probably work, nice :)

[gkistanova]

Hello Marcos,

I have commented couple things inline.

You may want to reconsider how you try to set the environments. What you are trying to do wouldn't work. Property('slave_env') is a local snapshot of the slave envs, and does not get propagated back to the slave.
You either would need to use a build step env param to set an env var, or do something else which would not require that.
At the first glance, Reid's suggestion of reducing the number of dependencies looks as a way to go, unless you have a strong reason to dial with DLLs.

zorg/buildbot/builders/SanitizerBuilderWindows.py

118

Depending on a build slave version, RemoveDirectory might not work well on Windows, if some directories are not empty or temporarily locked by some service or anti-virus. Just FYI.

Unfortunately, there is no good solution for this. Issuing "rmdir /S /Q" command might fail too, depending on the shell.

133

All these could be done simpler.

Every build step has a property "workdir" which contains a fully qualified path. When referencing a path, you could do something like

WithProperties(
     "-DCMAKE_CXX_COMPILER=%(workdir)s/" + staged_install + "/bin/clang++"
 ))

No need to be canonical with the directory separator as well, forward slashes would do just fine.

You can check how similar to this is done in the UnifiedTreeBuilder.py, lines 365-375.

150

All these could be done simpler.

Every build step has a property "workdir" which contains a fully qualified path. When referencing a path, you could do something like

WithProperties(
     "-DCMAKE_CXX_COMPILER=%(workdir)s/" + staged_install + "/bin/clang++"
 ))

No need to be canonical with the directory separator as well, forward slashes would do just fine.

You can check how similar to this is done in the UnifiedTreeBuilder.py, lines 365-375.

152

Could you use the CmakeCommand instead of the ShellCommand, please?

[mpividori]

@gkistanova @rnk Thanks for your feedback.
I have included suggested changes.
Unfortunately, I can't use MT for now. There is a problem with clang driver, and the order of static libraries. I will file a bug in bugzilla.
I still need to update the code to use CmakeCommand. Maybe in a different diff?

[rnk]

lgtm

Let's see if this Interpolate usage actually works, I guess. :)

[mpividori]

@gkistanova Do you agree?

[gkistanova]

We run buildbot 0.8.5 on the master. It does not have Interpolate.
So, please use WithProperties instead.

[mpividori]

@gkistanova Ok. Done. Let me know if you agree with the last diff.

[mpividori]

Ready to land?

[gkistanova]

Hello Marcos,

Feel free to commit after fixing that missing comma.

Would you make sure the slave for that builder will connect to the silent master and not production, to stage it for a day or two, please?

zorg/buildbot/builders/SanitizerBuilderWindows.py

131

You are missing a comma here between the arguments.

[gkistanova]

zorg/buildbot/builders/Util.py

23 ↗

(On Diff #87732)

A cosmetic - "and and"

[rnk]

In D29677#672429, @gkistanova wrote:

Hello Marcos,

Feel free to commit after fixing that missing comma.

Would you make sure the slave for that builder will connect to the silent master and not production, to stage it for a day or two, please?

The builder is already connected to the normal build master, though. Should we move it?

[gkistanova]

Yes, please, once this change is landed.
Just stop the slave, point it to the silent master, and start it again. It should connect with the same credentials.
In a day or two of the reliably green builds on the staging, feel free to return it back to the normal build master.

[zturner]

It seems less than ideal for our existing coverage to go silent if we connect this to the non production master.

What if we set haltOnFailure=False and flunkOnFailure=False for all of the newly added steps, until we can confirm that it's stable?

[gkistanova]

Hi Zachary,

This could do, if you are sure that these steps would not compromise the whole build.
Anyway, you can re-point the slave to the silent master if something would go wrong. Just be ready to act quickly. :)

[zturner]

Sounds good. Any chance of a master restart today after landing this, so that we can see by tomorrow morning if it's going to be a problem?

[gkistanova]

I'll restart the master at the end of the day today.

zorg/buildbot/builders/SanitizerBuilderWindows.py

 import os
+import glob
 
 import buildbot
 import buildbot.process.factory
 from buildbot.steps.shell import SetProperty
 from buildbot.steps.shell import ShellCommand
 from buildbot.steps.slave import RemoveDirectory
 from buildbot.steps.source import SVN
 from buildbot.process.properties import Property
def getSanitizerWindowsBuildFactory(
     f.addStep(SetProperty(
         command=getVisualStudioEnvironment(vs, target_arch),
         extract_fn=extractSlaveEnvironment))
 
     f = getSource(f,'llvm')
 
     # Global configurations
     build_dir='build'
+    build_fuzzer_dir='build-fuzzer'
 
     ############# CLEANING
     cleanBuildRequested = lambda step: step.build.getProperty("clean") or clean
     f.addStep(RemoveDirectory(name='clean '+build_dir,
                 dir=build_dir,
                 haltOnFailure=False,
                 flunkOnFailure=False,
                 doStepIf=cleanBuildRequested
test_targets = ['check-asan', 'check-asan-dynamic', 'check-sanitizer',
                     'check-cfi']
     f.addStep(NinjaCommand(name='run tests',
                            targets=test_targets,
                            haltOnFailure=True,
                            description='ninja test',
                            workdir=build_dir,
                            env=Property('slave_env')))
 
+    # Clean fuzzer build dir.
+    f.addStep(RemoveDirectory(name='clean '+build_fuzzer_dir,
+                dir=build_fuzzer_dir,
+                haltOnFailure=False,
+                flunkOnFailure=False,
+                doStepIf=cleanBuildRequested
+                ))
+
+    # Get absolute path to fresh clang.
+    config_path = os.path.abspath(os.path.join("..", build_dir, config))
+    bin_path = os.path.join(config_path, "bin")
+    dll_path = glob.glob(os.path.join(config_path, "lib", "clang", "*", "lib",
+                                      "windows"))[0]
+    clang_cl = os.path.join(bin_path, "clang-cl")
+
+    # Add fresh clang, tools and compiler-rt dlls to path.
+    Property('slave_env')['Path'] = ";".join([bin_path, dll_path,
+        Property('slave_env')['Path']])
+
+    f.addStep(ShellCommand(name='cmake',
+                           command=[cmake, "-G", "Ninja", "../llvm",
+                                    "-DCMAKE_BUILD_TYPE="+config,
+                                    "-DLLVM_ENABLE_ASSERTIONS=ON",
+                                    "-DCMAKE_C_COMPILER="+clang_cl,
+                                    "-DCMAKE_CXX_COMPILER="+clang_cl,
+                                    "-DLLVM_USE_SANITIZER=Address",
+                                    "-DLLVM_USE_SANITIZE_COVERAGE=YES"]
+                                   + extra_cmake_args,
+                           haltOnFailure=True,
+                           workdir=build_fuzzer_dir,
+                           env=Property('slave_env')))
+
+    # Build libFuzzer.
+    f.addStep(NinjaCommand(name='build LLVMFuzzer',
+                           targets=['LLVMFuzzer'],
+                           haltOnFailure=True,
+                           description='ninja LLVMFuzzer',
+                           workdir=build_dir,
+                           env=Property('slave_env')))
+
+    # Run libFuzzer's tests.
+    f.addStep(NinjaCommand(name='run fuzzer tests',
+                           targets=['check-fuzzer'],
+                           haltOnFailure=True,
+                           description='ninja check-fuzzer',
+                           workdir=build_fuzzer_dir,
+                           env=Property('slave_env')))
+
     return f