Skip to content

rdar://124230544 (Improve fidelity of missing input errors for dynamically requested inputs) #909

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 15, 2024
Merged

rdar://124230544 (Improve fidelity of missing input errors for dynamically requested inputs) #909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/llbuild/BuildSystem/BuildSystem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ class BuildSystemDelegate {
/// Called by the build system to report a command could not build due to
/// missing inputs.
virtual void commandCannotBuildOutputDueToMissingInputs(Command*,
Node* output, SmallPtrSet<Node*, 1> inputs) = 0;
Node* output, ArrayRef<BuildKey> inputs) = 0;

/// Called by the build system when a node has multiple commands that are producing it.
/// The delegate can return one of the commands for the build system to use or return \p nullptr
Expand Down
2 changes: 1 addition & 1 deletion include/llbuild/BuildSystem/BuildSystemFrontend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ class BuildSystemFrontendDelegate : public BuildSystemDelegate {
/// Called by the build system to report a command could not build due to
/// missing inputs.
virtual void commandCannotBuildOutputDueToMissingInputs(Command*,
Node* output, SmallPtrSet<Node*, 1> inputs) override;
Node* output, ArrayRef<BuildKey> inputs) override;

/// Called by the build system when a node has multiple commands that are producing it.
/// The delegate can return one of the commands for the build system to use or return \p nullptr
Expand Down
2 changes: 1 addition & 1 deletion include/llbuild/BuildSystem/Command.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ class Command : public basic::JobDescriptor {
const BuildValue& value) = 0;

virtual void provideValue(BuildSystem& system, core::TaskInterface,
uintptr_t inputID, const BuildValue& value) = 0;
uintptr_t inputID, const core::KeyType& key, const BuildValue& value) = 0;


virtual bool isExternalCommand() const { return false; }
Expand Down
7 changes: 3 additions & 4 deletions include/llbuild/BuildSystem/ExternalCommand.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

#include "llbuild/BuildSystem/BuildDescription.h"
#include "llbuild/BuildSystem/BuildSystem.h"
#include "llbuild/BuildSystem/BuildKey.h"
#include "llbuild/BuildSystem/BuildValue.h"
#include "llbuild/BuildSystem/Command.h"

Expand Down Expand Up @@ -66,10 +67,7 @@ class ExternalCommand : public Command {

/// If there are any elements, the command had missing input nodes
/// (this implies ShouldSkip is true).
SmallPtrSet<Node*, 1> missingInputNodes;

/// If true, the command had missing dynamic inputs.
bool hasMissingDynamicInputs = false;
SmallVector<BuildKey, 1> missingInputKeys;

/// If true, the command can legally be updated if the output state allows it.
bool canUpdateIfNewer = true;
Expand Down Expand Up @@ -143,6 +141,7 @@ class ExternalCommand : public Command {
virtual void provideValue(BuildSystem& system,
core::TaskInterface ti,
uintptr_t inputID,
const core::KeyType& key,
const BuildValue& value) override;

virtual void execute(BuildSystem& system,
Expand Down
2 changes: 1 addition & 1 deletion include/llbuild/Core/BuildEngine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ class Task {
///
/// \param value The computed value for the given input.
virtual void provideValue(TaskInterface, uintptr_t inputID,
const ValueType& value) = 0;
const KeyType& key, const ValueType& value) = 0;

/// Executed by the build engine to indicate that all inputs have been
/// provided, and the task should begin its computation. If the client will
Expand Down
37 changes: 20 additions & 17 deletions lib/BuildSystem/BuildSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,7 @@ class TargetTask : public Task {
}

virtual void provideValue(TaskInterface, uintptr_t inputID,
const ValueType& valueData) override {
const KeyType& key, const ValueType& valueData) override {
// Do nothing.
auto value = BuildValue::fromData(valueData);

Expand Down Expand Up @@ -484,7 +484,7 @@ class FileInputNodeTask : public Task {
}

virtual void provideValue(TaskInterface, uintptr_t inputID,
const ValueType& value) override {
const KeyType& key, const ValueType& value) override {
}

virtual void inputsAvailable(TaskInterface ti) override {
Expand Down Expand Up @@ -552,7 +552,7 @@ class StatTask : public Task {
}

virtual void provideValue(TaskInterface, uintptr_t inputID,
const ValueType& value) override {
const KeyType& key, const ValueType& value) override {
}

virtual void inputsAvailable(TaskInterface ti) override {
Expand Down Expand Up @@ -622,7 +622,7 @@ class DirectoryInputNodeTask : public Task {
}

virtual void provideValue(TaskInterface ti, uintptr_t inputID,
const ValueType& valueData) override {
const KeyType& key, const ValueType& valueData) override {
if (inputID == 0) {
directorySignature = valueData;
} else {
Expand Down Expand Up @@ -676,7 +676,7 @@ class DirectoryStructureInputNodeTask : public Task {
}

virtual void provideValue(TaskInterface, uintptr_t inputID,
const ValueType& value) override {
const KeyType& key, const ValueType& value) override {
directorySignature = value;
}

Expand All @@ -703,7 +703,7 @@ class VirtualInputNodeTask : public Task {
}

virtual void provideValue(TaskInterface, uintptr_t inputID,
const ValueType& value) override {
const KeyType& key, const ValueType& value) override {
}

virtual void inputsAvailable(TaskInterface ti) override {
Expand Down Expand Up @@ -768,7 +768,7 @@ class ProducedNodeTask : public Task {
}

virtual void provideValue(TaskInterface, uintptr_t inputID,
const ValueType& valueData) override {
const KeyType& key, const ValueType& valueData) override {
auto value = BuildValue::fromData(valueData);

// Extract the node result from the command.
Expand Down Expand Up @@ -852,7 +852,7 @@ class ProducedDirectoryNodeTask : public Task {
}

virtual void provideValue(TaskInterface ti, uintptr_t inputID,
const ValueType& valueData) override {
const KeyType& key, const ValueType& valueData) override {
if (inputID == 0) {
auto value = BuildValue::fromData(valueData);

Expand Down Expand Up @@ -962,7 +962,7 @@ class DirectoryContentsTask : public Task {
}

virtual void provideValue(TaskInterface, uintptr_t inputID,
const ValueType& value) override {
const KeyType& key, const ValueType& value) override {
if (inputID == 0) {
directoryValue = BuildValue::fromData(value);
return;
Expand Down Expand Up @@ -1131,7 +1131,7 @@ class FilteredDirectoryContentsTask : public Task {
}

virtual void provideValue(TaskInterface, uintptr_t inputID,
const ValueType& value) override {
const KeyType& key, const ValueType& value) override {
if (inputID == 1) {
directoryValue = BuildValue::fromData(value);
return;
Expand Down Expand Up @@ -1272,7 +1272,7 @@ class DirectoryTreeSignatureTask : public Task {
}

virtual void provideValue(TaskInterface ti, uintptr_t inputID,
const ValueType& valueData) override {
const KeyType& key, const ValueType& valueData) override {
// The first input is the directory contents.
if (inputID == 0) {
// Record the value for the directory.
Expand Down Expand Up @@ -1417,7 +1417,7 @@ class DirectoryTreeStructureSignatureTask : public Task {
}

virtual void provideValue(TaskInterface ti, uintptr_t inputID,
const ValueType& valueData) override {
const KeyType& key, const ValueType& valueData) override {
// The first input is the directory contents.
if (inputID == 0) {
// Record the value for the directory.
Expand Down Expand Up @@ -1539,9 +1539,9 @@ class CommandTask : public Task {
}

virtual void provideValue(TaskInterface ti, uintptr_t inputID,
const ValueType& valueData) override {
const KeyType& key, const ValueType& valueData) override {
command.provideValue(getBuildSystem(ti).getBuildSystem(), ti, inputID,
BuildValue::fromData(valueData));
key, BuildValue::fromData(valueData));
}

virtual void inputsAvailable(TaskInterface ti) override {
Expand Down Expand Up @@ -1609,7 +1609,7 @@ class MissingCommandTask : public Task {
const ValueType& valueData) override { }

virtual void provideValue(TaskInterface, uintptr_t inputID,
const ValueType& valueData) override { }
const KeyType& key, const ValueType& valueData) override { }

virtual void inputsAvailable(TaskInterface ti) override {
// A missing command always builds to an invalid value, and forces
Expand Down Expand Up @@ -2481,6 +2481,7 @@ class SwiftGetVersionCommand : public Command {
virtual void provideValue(BuildSystem&,
TaskInterface ti,
uintptr_t inputID,
const KeyType& key,
const BuildValue& value) override { }

virtual void execute(BuildSystem&,
Expand Down Expand Up @@ -2888,14 +2889,15 @@ class SwiftCompilerShellCommand : public ExternalCommand {
virtual void provideValue(BuildSystem& system,
TaskInterface ti,
uintptr_t inputID,
const KeyType& key,
const BuildValue& value) override {
// We can ignore the 'swift-get-version' input, it is just used to detect
// that we need to rebuild.
if (inputID == core::BuildEngine::kMaximumInputID - 1) {
return;
}

ExternalCommand::provideValue(system, ti, inputID, value);
ExternalCommand::provideValue(system, ti, inputID, key, value);
}

virtual void startExternalCommand(BuildSystem&, TaskInterface) override {
Expand Down Expand Up @@ -3322,7 +3324,7 @@ class SymlinkCommand : public Command {

virtual void provideValue(BuildSystem&, TaskInterface ti,
uintptr_t inputID,
const BuildValue& value) override {
const KeyType& key, const BuildValue& value) override {
assert(0 && "unexpected API call");
}

Expand Down Expand Up @@ -3864,6 +3866,7 @@ class StaleFileRemovalCommand : public Command {
virtual void provideValue(BuildSystem&,
TaskInterface,
uintptr_t inputID,
const KeyType& key,
const BuildValue& value) override {
assert(0 && "unexpected API call");
}
Expand Down
8 changes: 4 additions & 4 deletions lib/BuildSystem/BuildSystemFrontend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,19 +685,19 @@ void BuildSystemFrontendDelegate::commandFoundDiscoveredDependency(Command*, Str
}

void BuildSystemFrontendDelegate::commandCannotBuildOutputDueToMissingInputs(
Command * command, Node *output, SmallPtrSet<Node *, 1> inputs) {
Command * command, Node *output, ArrayRef<BuildKey> inputs) {
std::string message;
llvm::raw_string_ostream messageStream(message);

messageStream << "cannot build '";
messageStream << output->getName().str();
messageStream << "' due to missing inputs: ";

for (Node* input : inputs) {
if (input != *inputs.begin()) {
for (size_t i = 0; i < inputs.size(); ++i) {
if (i > 0) {
messageStream << ", ";
}
messageStream << "'" << input->getName() << "'";
messageStream << "'" << (inputs[i].isNode() ? inputs[i].getNodeName().str() : inputs[i].getKeyData().str()) << "'";
}

messageStream.flush();
Expand Down
16 changes: 7 additions & 9 deletions lib/BuildSystem/ExternalCommand.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,8 +219,7 @@ void ExternalCommand::start(BuildSystem& system,
core::TaskInterface ti) {
// Initialize the build state.
skipValue = llvm::None;
missingInputNodes.clear();
hasMissingDynamicInputs = false;
missingInputKeys.clear();

// Request all of the inputs.
unsigned id = 0;
Expand All @@ -243,6 +242,7 @@ void ExternalCommand::providePriorValue(BuildSystem& system,
void ExternalCommand::provideValue(BuildSystem& system,
core::TaskInterface ti,
uintptr_t inputID,
const core::KeyType& key,
const BuildValue& value) {
// Inform subclasses about the value
provideValueExternalCommand(system, ti, inputID, value);
Expand Down Expand Up @@ -306,10 +306,9 @@ void ExternalCommand::provideValue(BuildSystem& system,
skipValue = std::move(skipValueForInput);
if (value.isMissingInput()) {
if (inputs.size() > inputID) {
missingInputNodes.insert(inputs[inputID]);
missingInputKeys.push_back(BuildKey::makeNode(inputs[inputID]));
} else {
// FIXME: This should better track which dynamic input was missing. Right now, the higher level build system needs to reconstruct that information.
hasMissingDynamicInputs = true;
missingInputKeys.push_back(BuildKey::fromData(key));
}
}
} else {
Expand Down Expand Up @@ -380,9 +379,9 @@ void ExternalCommand::execute(BuildSystem& system,
// If this command should be skipped, do nothing.
if (skipValue.hasValue()) {
// If this command had a failed input, treat it as having failed.
if (!missingInputNodes.empty() || hasMissingDynamicInputs) {
if (!missingInputKeys.empty()) {
system.getDelegate().commandCannotBuildOutputDueToMissingInputs(this,
outputs.empty() ? nullptr : outputs[0], missingInputNodes);
outputs.empty() ? nullptr : outputs[0], missingInputKeys);

// Report the command failure.
system.getDelegate().hadCommandFailure();
Expand All @@ -391,8 +390,7 @@ void ExternalCommand::execute(BuildSystem& system,
resultFn(std::move(skipValue.getValue()));
return;
}
assert(missingInputNodes.empty());
assert(!hasMissingDynamicInputs);
assert(missingInputKeys.empty());

// If it is legal to simply update the command, then see if we can do so.
if (canUpdateIfNewer && hasPriorResult) {
Expand Down
2 changes: 1 addition & 1 deletion lib/Commands/BuildEngineCommand.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ struct AckermannTask : core::Task {

/// Called when a task’s requested input is available.
virtual void provideValue(core::TaskInterface ti, uintptr_t inputID,
const core::ValueType& value) override {
const core::KeyType& key, const core::ValueType& value) override {
if (inputID == 0) {
recursiveResultA = value;

Expand Down
1 change: 1 addition & 0 deletions lib/Commands/BuildSystemCommand.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ class ParseDummyCommand : public Command {
virtual void provideValue(BuildSystem&,
TaskInterface,
uintptr_t inputID,
const KeyType& key,
const BuildValue&) override {}
virtual void execute(BuildSystem& system, TaskInterface,
basic::QueueJobContext*, ResultFn resultFn) override {
Expand Down
8 changes: 4 additions & 4 deletions lib/Commands/NinjaBuildCommand.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -983,7 +983,7 @@ buildCommand(BuildContext& context, ninja::Command* command) {
}

virtual void provideValue(core::TaskInterface, uintptr_t inputID,
const core::ValueType& valueData) override {
const core::KeyType& key, const core::ValueType& valueData) override {
// Process the input value to see if we should skip this command.
BuildValue value = BuildValue::fromValue(valueData);

Expand Down Expand Up @@ -1469,7 +1469,7 @@ static core::Task* buildInput(BuildContext& context, ninja::Node* input) {
: context(context), node(node) { }

virtual void provideValue(core::TaskInterface, uintptr_t inputID,
const core::ValueType& value) override { }
const core::KeyType& key, const core::ValueType& value) override { }

virtual void start(core::TaskInterface) override { }

Expand Down Expand Up @@ -1504,7 +1504,7 @@ buildTargets(BuildContext& context,
: context(context), targetsToBuild(targetsToBuild) { }

virtual void provideValue(core::TaskInterface, uintptr_t inputID,
const core::ValueType& valueData) override {
const core::KeyType& key, const core::ValueType& valueData) override {
BuildValue value = BuildValue::fromValue(valueData);

if (value.isMissingInput()) {
Expand Down Expand Up @@ -1555,7 +1555,7 @@ selectCompositeBuildResult(BuildContext& context, ninja::Command* command,
}

virtual void provideValue(core::TaskInterface, uintptr_t inputID,
const core::ValueType& valueData) override {
const core::KeyType& key, const core::ValueType& valueData) override {
compositeValueData = &valueData;
}

Expand Down
2 changes: 1 addition & 1 deletion lib/Core/BuildEngine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -886,7 +886,7 @@ class BuildEngineImpl : public BuildDBDelegate {
TracingEngineTaskCallback i(EngineTaskCallbackKind::ProvideValue, request.inputRuleInfo->keyID);
TaskInterface iface{this, request.taskInfo->task.get()};
request.taskInfo->task->provideValue(
iface, request.inputID, request.inputRuleInfo->result.value);
iface, request.inputID, request.inputRuleInfo->rule->key, request.inputRuleInfo->result.value);
}

// Decrement the wait count, and move to finish queue if necessary.
Expand Down
4 changes: 2 additions & 2 deletions lib/Evo/EvoEngine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ class EvoTask : public core::Task, public EvoEngine {

// core::Task required methods
void start(core::TaskInterface) override;
void provideValue(core::TaskInterface, uintptr_t inputID, const core::ValueType& value) override;
void provideValue(core::TaskInterface, uintptr_t inputID, const core::KeyType& key, const core::ValueType& value) override;
void inputsAvailable(core::TaskInterface) override;

// EvoEngine methods
Expand Down Expand Up @@ -201,7 +201,7 @@ void EvoTask::start(core::TaskInterface ti) {
}

void EvoTask::provideValue(core::TaskInterface, uintptr_t inputID,
const core::ValueType& value) {
const core::KeyType& key, const core::ValueType& value) {
std::unique_lock<std::mutex> lock(taskMutex);
inputs[inputID].first = true;
inputs[inputID].second = value; // FIXME: avoid copying value ?
Expand Down
Loading