Skip to content

Commit

Permalink
[BuildSystem] Fix BuildValue initialization & serialization.
Browse files Browse the repository at this point in the history
 - The initializer was allowing uninitialized data in the commandSignature for
   sentinel values (like virtual inputs).

 - The move initialization was failing to initialize some fields (!).

 - The serialization was also failing to properly normalize values, so the
   serialized value was not properly deterministic.
  • Loading branch information
ddunbar committed Jan 18, 2017
1 parent aef909e commit 346849f
Show file tree
Hide file tree
Showing 5 changed files with 126 additions and 8 deletions.
21 changes: 15 additions & 6 deletions include/llbuild/BuildSystem/BuildValue.h
Expand Up @@ -100,6 +100,7 @@ class BuildValue {
BuildValue() {}
BuildValue(Kind kind) : kind(kind) {
valueData.asOutputInfo = {};
commandSignature = 0;
}
BuildValue(Kind kind, ArrayRef<FileInfo> outputInfos,
uint64_t commandSignature = 0)
Expand All @@ -121,14 +122,16 @@ class BuildValue {
// BuildValues can only be moved, not copied.
BuildValue(BuildValue&& rhs) : numOutputInfos(rhs.numOutputInfos) {
kind = rhs.kind;
if (numOutputInfos == 1) {
valueData.asOutputInfo = rhs.valueData.asOutputInfo;
} else {
numOutputInfos = rhs.numOutputInfos;
commandSignature = rhs.commandSignature;
if (rhs.hasMultipleOutputs()) {
valueData.asOutputInfos = rhs.valueData.asOutputInfos;
rhs.valueData.asOutputInfos = nullptr;
} else {
valueData.asOutputInfo = rhs.valueData.asOutputInfo;
}
}
BuildValue &operator=(BuildValue&& rhs) {
BuildValue& operator=(BuildValue&& rhs) {
if (this != &rhs) {
// Release our resources.
if (hasMultipleOutputs())
Expand All @@ -137,7 +140,8 @@ class BuildValue {
// Move the data.
kind = rhs.kind;
numOutputInfos = rhs.numOutputInfos;
if (numOutputInfos > 1) {
commandSignature = rhs.commandSignature;
if (rhs.hasMultipleOutputs()) {
valueData.asOutputInfos = rhs.valueData.asOutputInfos;
rhs.valueData.asOutputInfos = nullptr;
} else {
Expand Down Expand Up @@ -208,7 +212,9 @@ class BuildValue {
bool isFailedInput() const { return kind == Kind::FailedInput; }
bool isSuccessfulCommand() const {return kind == Kind::SuccessfulCommand; }
bool isFailedCommand() const { return kind == Kind::FailedCommand; }
bool isPropagatedFailureCommand() const { return kind == Kind::PropagatedFailureCommand; }
bool isPropagatedFailureCommand() const {
return kind == Kind::PropagatedFailureCommand;
}
bool isCancelledCommand() const { return kind == Kind::CancelledCommand; }
bool isSkippedCommand() const { return kind == Kind::SkippedCommand; }
bool isTarget() const { return kind == Kind::Target; }
Expand Down Expand Up @@ -279,6 +285,9 @@ class BuildValue {
std::vector<uint8_t> result(sizeof(*this) +
numOutputInfos * sizeof(FileInfo));
memcpy(result.data(), this, sizeof(*this));
// We must normalize the bytes where the address is stored.
memset(result.data() + offsetof(BuildValue, valueData), 0,
sizeof(valueData));
memcpy(result.data() + sizeof(*this), valueData.asOutputInfos,
numOutputInfos * sizeof(FileInfo));
return result;
Expand Down
2 changes: 1 addition & 1 deletion lib/BuildSystem/BuildSystem.cpp
Expand Up @@ -128,7 +128,7 @@ class BuildSystemEngineDelegate : public BuildEngineDelegate {

class BuildSystemImpl : public BuildSystemCommandInterface {
/// The internal schema version.
static const uint32_t internalSchemaVersion = 3;
static const uint32_t internalSchemaVersion = 4;

BuildSystem& buildSystem;

Expand Down
4 changes: 4 additions & 0 deletions llbuild.xcodeproj/project.pbxproj
Expand Up @@ -122,6 +122,7 @@
E1604CB51BB9E03E001153A1 /* swift-build-tool.cpp in Sources */ = {isa = PBXBuildFile; fileRef = E1604CB41BB9E032001153A1 /* swift-build-tool.cpp */; };
E171538D1A0BF702004CD598 /* CorePerfTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = E171538C1A0BF702004CD598 /* CorePerfTests.mm */; };
E17440C31CE192FF0070A30C /* ShellUtility.cpp in Sources */ = {isa = PBXBuildFile; fileRef = E17440C21CE192FF0070A30C /* ShellUtility.cpp */; };
E192E92F1E30014E00122F17 /* BuildValueTest.cpp in Sources */ = {isa = PBXBuildFile; fileRef = E192E92E1E30014E00122F17 /* BuildValueTest.cpp */; };
E19D79921A15D9E6002604FB /* MakefileDepsParser.cpp in Sources */ = {isa = PBXBuildFile; fileRef = E19D79911A15D9E6002604FB /* MakefileDepsParser.cpp */; };
E19D79951A15DA06002604FB /* MakefileDepsParserTest.cpp in Sources */ = {isa = PBXBuildFile; fileRef = E19D79941A15DA06002604FB /* MakefileDepsParserTest.cpp */; };
E1A0B0FF1C971582006DA08F /* DependencyInfoParser.cpp in Sources */ = {isa = PBXBuildFile; fileRef = E1A0B0FE1C971581006DA08F /* DependencyInfoParser.cpp */; };
Expand Down Expand Up @@ -742,6 +743,7 @@
E17C29F31B5AC2A700C12DA9 /* build-sphinx-docs.sh */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.script.sh; path = "build-sphinx-docs.sh"; sourceTree = "<group>"; };
E18043391A00129400662FE7 /* install-user-lit.sh */ = {isa = PBXFileReference; lastKnownFileType = text.script.sh; path = "install-user-lit.sh"; sourceTree = "<group>"; };
E182BE111ABA2B8D001840AD /* Compiler.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = Compiler.h; sourceTree = "<group>"; };
E192E92E1E30014E00122F17 /* BuildValueTest.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = BuildValueTest.cpp; sourceTree = "<group>"; };
E19C3F171B98B3CB0035E1AA /* extract-perf-data */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; path = "extract-perf-data"; sourceTree = "<group>"; };
E19C3F181B98B3CB0035E1AA /* xcs-submit-perf-data.sh */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.script.sh; path = "xcs-submit-perf-data.sh"; sourceTree = "<group>"; };
E19C3FD51B98C1A70035E1AA /* tests */ = {isa = PBXFileReference; lastKnownFileType = folder; path = tests; sourceTree = "<group>"; };
Expand Down Expand Up @@ -1177,6 +1179,7 @@
children = (
C5740D0D1E0352D800567DD8 /* CMakeLists.txt */,
C5740D081E03523100567DD8 /* BuildSystemFrontendTest.cpp */,
E192E92E1E30014E00122F17 /* BuildValueTest.cpp */,
9DB0478B1DF9D3E2006CDF52 /* LaneBasedExecutionQueueTest.cpp */,
9D0A6D7F1E1FFEA800BE636F /* TempDir.cpp */,
9D0A6D801E1FFEA800BE636F /* TempDir.hpp */,
Expand Down Expand Up @@ -2604,6 +2607,7 @@
isa = PBXSourcesBuildPhase;
buildActionMask = 2147483647;
files = (
E192E92F1E30014E00122F17 /* BuildValueTest.cpp in Sources */,
9DB047C01DF9F592006CDF52 /* LaneBasedExecutionQueueTest.cpp in Sources */,
C5740D091E03523100567DD8 /* BuildSystemFrontendTest.cpp in Sources */,
9D0A6D811E1FFEA800BE636F /* TempDir.cpp in Sources */,
Expand Down
104 changes: 104 additions & 0 deletions unittests/BuildSystem/BuildValueTest.cpp
@@ -0,0 +1,104 @@
//===- unittests/BuildSystem/BuildValueTest.cpp ---------------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2016 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See http://swift.org/LICENSE.txt for license information
// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//
//===----------------------------------------------------------------------===//

#include "llbuild/BuildSystem/BuildValue.h"

#include "gtest/gtest.h"

using namespace llbuild;
using namespace llbuild::buildsystem;
using namespace llvm;

namespace {

TEST(BuildValueTest, virtualValueSerialization) {
// Check that two identical values are equivalent.
{
BuildValue a = BuildValue::makeVirtualInput();
EXPECT_EQ(a.toData(), BuildValue::makeVirtualInput().toData());
}

// Check that a moved complex value is equivalent.
{
BuildValue tmp = BuildValue::makeVirtualInput();
BuildValue a = std::move(tmp);
EXPECT_EQ(a.toData(), BuildValue::makeVirtualInput().toData());
}

// Check that an rvalue initialized complex value is equivalent.
{
BuildValue tmp = BuildValue::makeVirtualInput();
BuildValue a{ std::move(tmp) };
EXPECT_EQ(a.toData(), BuildValue::makeVirtualInput().toData());
}
}

TEST(BuildValueTest, commandValueSingleOutputSerialization) {
uint64_t signature = 0xDEADBEEF;
basic::FileInfo infos[1] = {};
infos[0].size = 1;

// Check that two identical values are equivalent.
{
BuildValue a = BuildValue::makeSuccessfulCommand(infos, signature);
EXPECT_EQ(a.toData(),
BuildValue::makeSuccessfulCommand(infos, signature).toData());
}

// Check that a moved complex value is equivalent.
{
BuildValue tmp = BuildValue::makeSuccessfulCommand(infos, signature);
BuildValue a = std::move(tmp);
EXPECT_EQ(a.toData(),
BuildValue::makeSuccessfulCommand(infos, signature).toData());
}

// Check that an rvalue initialized complex value is equivalent.
{
BuildValue tmp = BuildValue::makeSuccessfulCommand(infos, signature);
BuildValue a{ std::move(tmp) };
EXPECT_EQ(a.toData(),
BuildValue::makeSuccessfulCommand(infos, signature).toData());
}
}

TEST(BuildValueTest, commandValueMultipleOutputsSerialization) {
uint64_t signature = 0xDEADBEEF;
basic::FileInfo infos[2] = {};
infos[0].size = 1;
infos[1].size = 2;

// Check that two identical values are equivalent.
{
BuildValue a = BuildValue::makeSuccessfulCommand(infos, signature);
EXPECT_EQ(a.toData(),
BuildValue::makeSuccessfulCommand(infos, signature).toData());
}

// Check that a moved complex value is equivalent.
{
BuildValue tmp = BuildValue::makeSuccessfulCommand(infos, signature);
BuildValue a = std::move(tmp);
EXPECT_EQ(a.toData(),
BuildValue::makeSuccessfulCommand(infos, signature).toData());
}

// Check that an rvalue initialized complex value is equivalent.
{
BuildValue tmp = BuildValue::makeSuccessfulCommand(infos, signature);
BuildValue a{ std::move(tmp) };
EXPECT_EQ(a.toData(),
BuildValue::makeSuccessfulCommand(infos, signature).toData());
}
}

}
3 changes: 2 additions & 1 deletion unittests/BuildSystem/CMakeLists.txt
@@ -1,6 +1,7 @@
add_llbuild_unittest(BuildSystemTests
LaneBasedExecutionQueueTest
BuildSystemFrontendTest.cpp
BuildValueTest.cpp
LaneBasedExecutionQueueTest
TempDir.cpp
)

Expand Down

0 comments on commit 346849f

Please sign in to comment.