Skip to content

Commit

Permalink
[FileCheck] Better diagnostic for format conflict
Browse files Browse the repository at this point in the history 
Summary:
Improve error message in case of conflict between several implicit
format to mention the operand that conflict.

Reviewers: jhenderson, jdenny, probinson, grimar, arichardson, rnk

Reviewed By: jdenny

Subscribers: hiraditya, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D77741
  • Loading branch information
Thomas Preud'homme committed Apr 15, 2020
1 parent 2a0a26b commit 9743123
Show file tree
Hide file tree
Showing 4 changed files with 276 additions and 146 deletions.
111 changes: 71 additions & 40 deletions llvm/lib/Support/FileCheck.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,20 @@

using namespace llvm;

StringRef ExpressionFormat::toString() const {
switch (Value) {
case Kind::NoFormat:
return StringRef("<none>");
case Kind::Unsigned:
return StringRef("%u");
case Kind::HexUpper:
return StringRef("%X");
case Kind::HexLower:
return StringRef("%x");
}
llvm_unreachable("unknown expression format");
}

Expected<StringRef> ExpressionFormat::getWildcardRegex() const {
switch (Value) {
case Kind::Unsigned:
Expand Down Expand Up @@ -71,7 +85,7 @@ Expected<uint64_t> NumericVariableUse::eval() const {
if (Value)
return *Value;

return make_error<UndefVarError>(Name);
return make_error<UndefVarError>(getExpressionStr());
}

Expected<uint64_t> BinaryOperation::eval() const {
Expand All @@ -92,18 +106,31 @@ Expected<uint64_t> BinaryOperation::eval() const {
return EvalBinop(*LeftOp, *RightOp);
}

ExpressionFormat BinaryOperation::getImplicitFormat() const {
ExpressionFormat LeftFormat = LeftOperand->getImplicitFormat();
ExpressionFormat RightFormat = RightOperand->getImplicitFormat();

ExpressionFormat Format =
LeftFormat != ExpressionFormat::Kind::NoFormat ? LeftFormat : RightFormat;
if (LeftFormat != ExpressionFormat::Kind::NoFormat &&
RightFormat != ExpressionFormat::Kind::NoFormat &&
LeftFormat != RightFormat)
Format = ExpressionFormat(ExpressionFormat::Kind::Conflict);
Expected<ExpressionFormat>
BinaryOperation::getImplicitFormat(const SourceMgr &SM) const {
Expected<ExpressionFormat> LeftFormat = LeftOperand->getImplicitFormat(SM);
Expected<ExpressionFormat> RightFormat = RightOperand->getImplicitFormat(SM);
if (!LeftFormat || !RightFormat) {
Error Err = Error::success();
if (!LeftFormat)
Err = joinErrors(std::move(Err), LeftFormat.takeError());
if (!RightFormat)
Err = joinErrors(std::move(Err), RightFormat.takeError());
return std::move(Err);
}

return Format;
if (*LeftFormat != ExpressionFormat::Kind::NoFormat &&
*RightFormat != ExpressionFormat::Kind::NoFormat &&
*LeftFormat != *RightFormat)
return ErrorDiagnostic::get(
SM, getExpressionStr(),
"implicit format conflict between '" + LeftOperand->getExpressionStr() +
"' (" + LeftFormat->toString() + ") and '" +
RightOperand->getExpressionStr() + "' (" + RightFormat->toString() +
"), need an explicit format specifier");

return *LeftFormat != ExpressionFormat::Kind::NoFormat ? *LeftFormat
: *RightFormat;
}

Expected<std::string> NumericSubstitution::getResult() const {
Expand Down Expand Up @@ -262,9 +289,12 @@ Expected<std::unique_ptr<ExpressionAST>> Pattern::parseNumericOperand(

// Otherwise, parse it as a literal.
uint64_t LiteralValue;
StringRef OperandExpr = Expr;
if (!Expr.consumeInteger((AO == AllowedOperand::LegacyLiteral) ? 10 : 0,
LiteralValue))
return std::make_unique<ExpressionLiteral>(LiteralValue);
LiteralValue)) {
return std::make_unique<ExpressionLiteral>(
OperandExpr.drop_back(Expr.size()), LiteralValue);
}

return ErrorDiagnostic::get(SM, Expr,
"invalid operand format '" + Expr + "'");
Expand All @@ -279,17 +309,18 @@ static uint64_t sub(uint64_t LeftOp, uint64_t RightOp) {
}

Expected<std::unique_ptr<ExpressionAST>>
Pattern::parseBinop(StringRef &Expr, std::unique_ptr<ExpressionAST> LeftOp,
Pattern::parseBinop(StringRef Expr, StringRef &RemainingExpr,
std::unique_ptr<ExpressionAST> LeftOp,
bool IsLegacyLineExpr, Optional<size_t> LineNumber,
FileCheckPatternContext *Context, const SourceMgr &SM) {
Expr = Expr.ltrim(SpaceChars);
if (Expr.empty())
RemainingExpr = RemainingExpr.ltrim(SpaceChars);
if (RemainingExpr.empty())
return std::move(LeftOp);

// Check if this is a supported operation and select a function to perform
// it.
SMLoc OpLoc = SMLoc::getFromPointer(Expr.data());
char Operator = popFront(Expr);
SMLoc OpLoc = SMLoc::getFromPointer(RemainingExpr.data());
char Operator = popFront(RemainingExpr);
binop_eval_t EvalBinop;
switch (Operator) {
case '+':
Expand All @@ -304,19 +335,20 @@ Pattern::parseBinop(StringRef &Expr, std::unique_ptr<ExpressionAST> LeftOp,
}

// Parse right operand.
Expr = Expr.ltrim(SpaceChars);
if (Expr.empty())
return ErrorDiagnostic::get(SM, Expr, "missing operand in expression");
RemainingExpr = RemainingExpr.ltrim(SpaceChars);
if (RemainingExpr.empty())
return ErrorDiagnostic::get(SM, RemainingExpr,
"missing operand in expression");
// The second operand in a legacy @LINE expression is always a literal.
AllowedOperand AO =
IsLegacyLineExpr ? AllowedOperand::LegacyLiteral : AllowedOperand::Any;
Expected<std::unique_ptr<ExpressionAST>> RightOpResult =
parseNumericOperand(Expr, AO, LineNumber, Context, SM);
parseNumericOperand(RemainingExpr, AO, LineNumber, Context, SM);
if (!RightOpResult)
return RightOpResult;

Expr = Expr.ltrim(SpaceChars);
return std::make_unique<BinaryOperation>(EvalBinop, std::move(LeftOp),
Expr = Expr.drop_back(RemainingExpr.size());
return std::make_unique<BinaryOperation>(Expr, EvalBinop, std::move(LeftOp),
std::move(*RightOpResult));
}

Expand Down Expand Up @@ -370,17 +402,18 @@ Expected<std::unique_ptr<Expression>> Pattern::parseNumericSubstitutionBlock(

// Parse the expression itself.
Expr = Expr.ltrim(SpaceChars);
StringRef UseExpr = Expr;
if (!Expr.empty()) {
Expr = Expr.rtrim(SpaceChars);
StringRef OuterBinOpExpr = Expr;
// The first operand in a legacy @LINE expression is always the @LINE
// pseudo variable.
AllowedOperand AO =
IsLegacyLineExpr ? AllowedOperand::LineVar : AllowedOperand::Any;
Expected<std::unique_ptr<ExpressionAST>> ParseResult =
parseNumericOperand(Expr, AO, LineNumber, Context, SM);
while (ParseResult && !Expr.empty()) {
ParseResult = parseBinop(Expr, std::move(*ParseResult), IsLegacyLineExpr,
LineNumber, Context, SM);
ParseResult = parseBinop(OuterBinOpExpr, Expr, std::move(*ParseResult),
IsLegacyLineExpr, LineNumber, Context, SM);
// Legacy @LINE expressions only allow 2 operands.
if (ParseResult && IsLegacyLineExpr && !Expr.empty())
return ErrorDiagnostic::get(
Expand All @@ -396,19 +429,17 @@ Expected<std::unique_ptr<Expression>> Pattern::parseNumericSubstitutionBlock(
// otherwise (ii) its implicit format, if any, otherwise (iii) the default
// format (unsigned). Error out in case of conflicting implicit format
// without explicit format.
ExpressionFormat Format,
ImplicitFormat = ExpressionASTPointer
? ExpressionASTPointer->getImplicitFormat()
: ExpressionFormat(ExpressionFormat::Kind::NoFormat);
if (bool(ExplicitFormat))
ExpressionFormat Format;
if (ExplicitFormat)
Format = ExplicitFormat;
else if (ImplicitFormat == ExpressionFormat::Kind::Conflict)
return ErrorDiagnostic::get(
SM, UseExpr,
"variables with conflicting format specifier: need an explicit one");
else if (bool(ImplicitFormat))
Format = ImplicitFormat;
else
else if (ExpressionASTPointer) {
Expected<ExpressionFormat> ImplicitFormat =
ExpressionASTPointer->getImplicitFormat(SM);
if (!ImplicitFormat)
return ImplicitFormat.takeError();
Format = *ImplicitFormat;
}
if (!Format)
Format = ExpressionFormat(ExpressionFormat::Kind::Unsigned);

std::unique_ptr<Expression> ExpressionPointer =
Expand Down
84 changes: 48 additions & 36 deletions llvm/lib/Support/FileCheckImpl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,6 @@ struct ExpressionFormat {
/// Denote absence of format. Used for implicit format of literals and
/// empty expressions.
NoFormat,
/// Used when there are several conflicting implicit formats in an
/// expression.
Conflict,
/// Value is an unsigned integer and should be printed as a decimal number.
Unsigned,
/// Value should be printed as an uppercase hex number.
Expand All @@ -55,9 +52,7 @@ struct ExpressionFormat {

public:
/// Evaluates a format to true if it can be used in a match.
explicit operator bool() const {
return Value != Kind::NoFormat && Value != Kind::Conflict;
}
explicit operator bool() const { return Value != Kind::NoFormat; }

/// Define format equality: formats are equal if neither is NoFormat and
/// their kinds are the same.
Expand All @@ -73,16 +68,19 @@ struct ExpressionFormat {

bool operator!=(Kind OtherValue) const { return !(*this == OtherValue); }

/// \returns the format specifier corresponding to this format as a string.
StringRef toString() const;

ExpressionFormat() : Value(Kind::NoFormat){};
explicit ExpressionFormat(Kind Value) : Value(Value){};

/// \returns a wildcard regular expression StringRef that matches any value
/// in the format represented by this instance, or an error if the format is
/// NoFormat or Conflict.
/// NoFormat.
Expected<StringRef> getWildcardRegex() const;

/// \returns the string representation of \p Value in the format represented
/// by this instance, or an error if the format is NoFormat or Conflict.
/// by this instance, or an error if the format is NoFormat.
Expected<std::string> getMatchingString(uint64_t Value) const;

/// \returns the value corresponding to string representation \p StrVal
Expand All @@ -95,17 +93,26 @@ struct ExpressionFormat {

/// Base class representing the AST of a given expression.
class ExpressionAST {
private:
StringRef ExpressionStr;

public:
ExpressionAST(StringRef ExpressionStr) : ExpressionStr(ExpressionStr) {}

virtual ~ExpressionAST() = default;

StringRef getExpressionStr() const { return ExpressionStr; }

/// Evaluates and \returns the value of the expression represented by this
/// AST or an error if evaluation fails.
virtual Expected<uint64_t> eval() const = 0;

/// \returns either the implicit format of this AST, FormatConflict if
/// implicit formats of the AST's components conflict, or NoFormat if the AST
/// has no implicit format (e.g. AST is made up of a single literal).
virtual ExpressionFormat getImplicitFormat() const {
/// \returns either the implicit format of this AST, a diagnostic against
/// \p SM if implicit formats of the AST's components conflict, or NoFormat
/// if the AST has no implicit format (e.g. AST is made up of a single
/// literal).
virtual Expected<ExpressionFormat>
getImplicitFormat(const SourceMgr &SM) const {
return ExpressionFormat();
}
};
Expand All @@ -117,8 +124,10 @@ class ExpressionLiteral : public ExpressionAST {
uint64_t Value;

public:
/// Constructs a literal with the specified value.
ExpressionLiteral(uint64_t Val) : Value(Val) {}
/// Constructs a literal with the specified value parsed from
/// \p ExpressionStr.
ExpressionLiteral(StringRef ExpressionStr, uint64_t Val)
: ExpressionAST(ExpressionStr), Value(Val) {}

/// \returns the literal's value.
Expected<uint64_t> eval() const override { return Value; }
Expand Down Expand Up @@ -222,21 +231,18 @@ class NumericVariable {
/// expression.
class NumericVariableUse : public ExpressionAST {
private:
/// Name of the numeric variable.
StringRef Name;

/// Pointer to the class instance for the variable this use is about.
NumericVariable *Variable;

public:
NumericVariableUse(StringRef Name, NumericVariable *Variable)
: Name(Name), Variable(Variable) {}

: ExpressionAST(Name), Variable(Variable) {}
/// \returns the value of the variable referenced by this instance.
Expected<uint64_t> eval() const override;

/// \returns implicit format of this numeric variable.
ExpressionFormat getImplicitFormat() const override {
Expected<ExpressionFormat>
getImplicitFormat(const SourceMgr &SM) const override {
return Variable->getImplicitFormat();
}
};
Expand All @@ -257,9 +263,10 @@ class BinaryOperation : public ExpressionAST {
binop_eval_t EvalBinop;

public:
BinaryOperation(binop_eval_t EvalBinop, std::unique_ptr<ExpressionAST> LeftOp,
BinaryOperation(StringRef ExpressionStr, binop_eval_t EvalBinop,
std::unique_ptr<ExpressionAST> LeftOp,
std::unique_ptr<ExpressionAST> RightOp)
: EvalBinop(EvalBinop) {
: ExpressionAST(ExpressionStr), EvalBinop(EvalBinop) {
LeftOperand = std::move(LeftOp);
RightOperand = std::move(RightOp);
}
Expand All @@ -270,10 +277,12 @@ class BinaryOperation : public ExpressionAST {
/// variable is used in one of the operands.
Expected<uint64_t> eval() const override;

/// \returns the implicit format of this AST, if any, a format conflict if
/// the implicit formats of the AST's components conflict, or no format if
/// the AST has no implicit format (e.g. AST is made of a single literal).
ExpressionFormat getImplicitFormat() const override;
/// \returns the implicit format of this AST, if any, a diagnostic against
/// \p SM if the implicit formats of the AST's components conflict, or no
/// format if the AST has no implicit format (e.g. AST is made of a single
/// literal).
Expected<ExpressionFormat>
getImplicitFormat(const SourceMgr &SM) const override;
};

class FileCheckPatternContext;
Expand Down Expand Up @@ -663,17 +672,20 @@ class Pattern {
parseNumericOperand(StringRef &Expr, AllowedOperand AO,
Optional<size_t> LineNumber,
FileCheckPatternContext *Context, const SourceMgr &SM);
/// Parses \p Expr for a binary operation at line \p LineNumber, or before
/// input is parsed if \p LineNumber is None. The left operand of this binary
/// operation is given in \p LeftOp and \p IsLegacyLineExpr indicates whether
/// we are parsing a legacy @LINE expression. Parameter \p Context points to
/// the class instance holding the live string and numeric variables.
/// \returns the class representing the binary operation in the AST of the
/// expression, or an error holding a diagnostic against \p SM otherwise.
/// Parses and updates \p RemainingExpr for a binary operation at line
/// \p LineNumber, or before input is parsed if \p LineNumber is None. The
/// left operand of this binary operation is given in \p LeftOp and \p Expr
/// holds the string for the full expression, including the left operand.
/// Parameter \p IsLegacyLineExpr indicates whether we are parsing a legacy
/// @LINE expression. Parameter \p Context points to the class instance
/// holding the live string and numeric variables. \returns the class
/// representing the binary operation in the AST of the expression, or an
/// error holding a diagnostic against \p SM otherwise.
static Expected<std::unique_ptr<ExpressionAST>>
parseBinop(StringRef &Expr, std::unique_ptr<ExpressionAST> LeftOp,
bool IsLegacyLineExpr, Optional<size_t> LineNumber,
FileCheckPatternContext *Context, const SourceMgr &SM);
parseBinop(StringRef Expr, StringRef &RemainingExpr,
std::unique_ptr<ExpressionAST> LeftOp, bool IsLegacyLineExpr,
Optional<size_t> LineNumber, FileCheckPatternContext *Context,
const SourceMgr &SM);
};

//===----------------------------------------------------------------------===//
Expand Down
25 changes: 18 additions & 7 deletions llvm/test/FileCheck/numeric-expression.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,16 +185,27 @@ CHECK-NEXT: [[# %u, VAR3]]

; Conflicting implicit format.
RUN: %ProtectFileCheckOutput \
RUN: not FileCheck --check-prefixes CHECK,FMT-CONFLICT --input-file %s %s 2>&1 \
RUN: | FileCheck --strict-whitespace --check-prefix FMT-CONFLICT-MSG %s
RUN: not FileCheck --check-prefixes CHECK,FMT-CONFLICT1 --input-file %s %s 2>&1 \
RUN: | FileCheck --strict-whitespace --check-prefix FMT-CONFLICT1-MSG %s
RUN: %ProtectFileCheckOutput \
RUN: not FileCheck --check-prefixes CHECK,FMT-CONFLICT2 --input-file %s %s 2>&1 \
RUN: | FileCheck --strict-whitespace --check-prefix FMT-CONFLICT2-MSG %s

VAR USE IMPL FMT CONFLICT
23
FMT-CONFLICT-LABEL: VAR USE IMPL FMT CONFLICT
FMT-CONFLICT-NEXT: [[#VAR1 + VAR2]]
FMT-CONFLICT-MSG: numeric-expression.txt:[[#@LINE-1]]:23: error: variables with conflicting format specifier: need an explicit one
FMT-CONFLICT-MSG-NEXT: {{F}}MT-CONFLICT-NEXT: {{\[\[#VAR1 \+ VAR2\]\]}}
FMT-CONFLICT-MSG-NEXT: {{^ \^$}}
FMT-CONFLICT1-LABEL: VAR USE IMPL FMT CONFLICT
FMT-CONFLICT1-NEXT: [[#VAR1 + VAR2]]
FMT-CONFLICT1-MSG: numeric-expression.txt:[[#@LINE-1]]:24: error: implicit format conflict between 'VAR1' (%u) and 'VAR2' (%x), need an explicit format specifier
FMT-CONFLICT1-MSG-NEXT: {{F}}MT-CONFLICT1-NEXT: {{\[\[#VAR1 \+ VAR2\]\]}}
FMT-CONFLICT1-MSG-NEXT: {{^ \^$}}

VAR USE IMPL FMT CONFLICT COMPLEX
34
FMT-CONFLICT2-LABEL: VAR USE IMPL FMT CONFLICT
FMT-CONFLICT2-NEXT: [[#VAR1 + VAR1a + VAR2]]
FMT-CONFLICT2-MSG: numeric-expression.txt:[[#@LINE-1]]:24: error: implicit format conflict between 'VAR1 + VAR1a' (%u) and 'VAR2' (%x), need an explicit format specifier
FMT-CONFLICT2-MSG-NEXT: {{F}}MT-CONFLICT2-NEXT: {{\[\[#VAR1 \+ VAR1a \+ VAR2\]\]}}
FMT-CONFLICT2-MSG-NEXT: {{^ \^$}}

; Explicitly specified format can override conflicting implicit formats.
VAR USE IMPL OVERRIDE FMT CONFLICT
Expand Down

0 comments on commit 9743123

Please sign in to comment.