Skip to content

Commit 343428c

Browse files
MaskRayMaskRay
committed
MC: Detect cyclic dependency for variable symbols
We report cyclic dependency errors for variable symbols and rely on isSymbolUsedInExpression in parseAssignmentExpression at parse time, which does not catch all setVariableValue cases (e.g. cyclic .weakref). Instead, add a bit to MCSymbol and check it when walking the variable value MCExpr. When a cycle is detected when we have a final layout, report an error and set the variable to a constant to avoid duplicate errors. isSymbolUsedInExpression is considered deprecated, but it is still used by AMDGPU (llvm#112251).
1 parent 4cb25e2 commit 343428c

File tree

8 files changed

+76
-30
lines changed

8 files changed

+76
-30
lines changed

‎llvm/include/llvm/MC/MCSymbol.h

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,9 @@ class MCSymbol {
112112
/// True if we have created a relocation that uses this symbol.
113113
mutable unsigned IsUsedInReloc : 1;
114114

115+
/// Used to detect cyclic dependency like `a = a + 1` and `a = b; b = a`.
116+
unsigned IsResolving : 1;
117+
115118
/// This is actually a Contents enumerator, but is unsigned to avoid sign
116119
/// extension and achieve better bitpacking with MSVC.
117120
unsigned SymbolContents : 3;
@@ -164,7 +167,7 @@ class MCSymbol {
164167
MCSymbol(SymbolKind Kind, const MCSymbolTableEntry *Name, bool isTemporary)
165168
: IsTemporary(isTemporary), IsRedefinable(false), IsUsed(false),
166169
IsRegistered(false), IsExternal(false), IsPrivateExtern(false),
167-
IsWeakExternal(false), Kind(Kind), IsUsedInReloc(false),
170+
IsWeakExternal(false), Kind(Kind), IsUsedInReloc(false), IsResolving(0),
168171
SymbolContents(SymContentsUnset), CommonAlignLog2(0), Flags(0) {
169172
Offset = 0;
170173
HasName = !!Name;
@@ -240,6 +243,9 @@ class MCSymbol {
240243
}
241244
}
242245

246+
bool isResolving() const { return IsResolving; }
247+
void setIsResolving(bool V) { IsResolving = V; }
248+
243249
/// @}
244250
/// \name Associated Sections
245251
/// @{

‎llvm/lib/MC/MCExpr.cpp

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
//===----------------------------------------------------------------------===//
88

99
#include "llvm/MC/MCExpr.h"
10+
#include "llvm/ADT/ScopeExit.h"
1011
#include "llvm/ADT/Statistic.h"
1112
#include "llvm/Config/llvm-config.h"
1213
#include "llvm/MC/MCAsmBackend.h"
@@ -497,13 +498,25 @@ bool MCExpr::evaluateAsRelocatableImpl(MCValue &Res, const MCAssembler *Asm,
497498

498499
case SymbolRef: {
499500
const MCSymbolRefExpr *SRE = cast<MCSymbolRefExpr>(this);
500-
const MCSymbol &Sym = SRE->getSymbol();
501+
MCSymbol &Sym = const_cast<MCSymbol &>(SRE->getSymbol());
501502
const auto Kind = SRE->getKind();
502503
bool Layout = Asm && Asm->hasLayout();
503504

504505
// Evaluate recursively if this is a variable.
506+
if (Sym.isResolving()) {
507+
if (Asm && Asm->hasFinalLayout()) {
508+
Asm->getContext().reportError(
509+
Sym.getVariableValue()->getLoc(),
510+
"cyclic dependency detected for symbol '" + Sym.getName() + "'");
511+
Sym.IsUsed = false;
512+
Sym.setVariableValue(MCConstantExpr::create(0, Asm->getContext()));
513+
}
514+
return false;
515+
}
505516
if (Sym.isVariable() && (Kind == MCSymbolRefExpr::VK_None || Layout) &&
506517
canExpand(Sym, InSet)) {
518+
Sym.setIsResolving(true);
519+
auto _ = make_scope_exit([&] { Sym.setIsResolving(false); });
507520
bool IsMachO =
508521
Asm && Asm->getContext().getAsmInfo()->hasSubsectionsViaSymbols();
509522
if (Sym.getVariableValue()->evaluateAsRelocatableImpl(Res, Asm,
@@ -709,9 +722,16 @@ MCFragment *MCExpr::findAssociatedFragment() const {
709722
return MCSymbol::AbsolutePseudoFragment;
710723

711724
case SymbolRef: {
712-
const MCSymbolRefExpr *SRE = cast<MCSymbolRefExpr>(this);
713-
const MCSymbol &Sym = SRE->getSymbol();
714-
return Sym.getFragment();
725+
auto &Sym =
726+
const_cast<MCSymbol &>(cast<MCSymbolRefExpr>(this)->getSymbol());
727+
if (Sym.Fragment)
728+
return Sym.Fragment;
729+
if (Sym.isResolving())
730+
return MCSymbol::AbsolutePseudoFragment;
731+
Sym.setIsResolving(true);
732+
auto *F = Sym.getFragment();
733+
Sym.setIsResolving(false);
734+
return F;
715735
}
716736

717737
case Unary:

‎llvm/lib/MC/MCParser/AsmParser.cpp

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6348,9 +6348,7 @@ bool parseAssignmentExpression(StringRef Name, bool allow_redef,
63486348
//
63496349
// FIXME: Diagnostics. Note the location of the definition as a label.
63506350
// FIXME: Diagnose assignment to protected identifier (e.g., register name).
6351-
if (Value->isSymbolUsedInExpression(Sym))
6352-
return Parser.Error(EqualLoc, "Recursive use of '" + Name + "'");
6353-
else if (Sym->isUndefined() && !Sym->isUsed() && !Sym->isVariable())
6351+
if (Sym->isUndefined() && !Sym->isUsed() && !Sym->isVariable())
63546352
; // Allow redefinitions of undefined symbols only used in directives.
63556353
else if (Sym->isVariable() && !Sym->isUsed() && allow_redef)
63566354
; // Allow redefinitions of variables that haven't yet been used.

‎llvm/test/MC/ARM/thumb_set-diagnostics.s

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
1-
@ RUN: not llvm-mc -triple armv7-eabi -o /dev/null 2>&1 %s | FileCheck %s
1+
@ RUN: not llvm-mc -triple armv7-eabi -o /dev/null %s 2>&1 | FileCheck %s
2+
@ RUN: not llvm-mc -filetype=obj -triple=armv7-eabi -o /dev/null %s --defsym LOOP=1 2>&1 | FileCheck %s --check-prefix=ERR2 --implicit-check-not=error:
23

34
.syntax unified
45

56
.thumb
67

8+
.ifndef LOOP
79
.thumb_set
810

911
@ CHECK: error: expected identifier after '.thumb_set'
@@ -52,13 +54,6 @@ beta:
5254

5355
@ CHECK: error: redefinition of 'beta'
5456
@ CHECK: .thumb_set beta, alpha
55-
@ CHECK: ^
56-
57-
.type recursive_use,%function
58-
.thumb_set recursive_use, recursive_use + 1
59-
60-
@ CHECK: error: Recursive use of 'recursive_use'
61-
@ CHECK: .thumb_set recursive_use, recursive_use + 1
6257
@ CHECK: ^
6358

6459
variable_result = alpha + 1
@@ -68,3 +63,15 @@ beta:
6863
@ CHECK: error: invalid reassignment of non-absolute variable 'variable_result'
6964
@ CHECK: .thumb_set variable_result, 1
7065
@ CHECK: ^
66+
67+
.else
68+
.type recursive_use,%function
69+
.thumb_set recursive_use, recursive_use + 1
70+
@ ERR2: [[#@LINE-1]]:41: error: cyclic dependency detected for symbol 'recursive_use'
71+
@ ERR2: [[#@LINE-2]]:41: error: expression could not be evaluated
72+
73+
.type recursive_use2,%function
74+
.thumb_set recursive_use2, recursive_use2 + 1
75+
@ ERR2: [[#@LINE-1]]:43: error: cyclic dependency detected for symbol 'recursive_use2'
76+
@ ERR2: [[#@LINE-2]]:43: error: expression could not be evaluated
77+
.endif

‎llvm/test/MC/AsmParser/equate-cycle.s

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
# RUN: not llvm-mc -filetype=obj %s -o /dev/null 2>&1 | FileCheck %s
2+
3+
# CHECK: [[#@LINE+2]]:7: error: cyclic dependency detected for symbol 'a'
4+
# CHECK: [[#@LINE+1]]:7: error: expression could not be evaluated
5+
a = a + 1
6+
7+
# CHECK: [[#@LINE+3]]:6: error: cyclic dependency detected for symbol 'b1'
8+
# CHECK: [[#@LINE+1]]:6: error: expression could not be evaluated
9+
b0 = b1
10+
b1 = b2
11+
b2 = b0
12+
13+
# CHECK: [[#@LINE+3]]:6: error: cyclic dependency detected for symbol 'c1'
14+
# CHECK: [[#@LINE+1]]:9: error: expression could not be evaluated
15+
c0 = c1 + 1
16+
c1 = c0

‎llvm/test/MC/AsmParser/variables-invalid.s

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
1-
// RUN: not llvm-mc -triple i386-unknown-unknown %s 2> %t
1+
// RUN: not llvm-mc -triple=x86_64 %s 2> %t
22
// RUN: FileCheck --input-file %t %s
33

44
.data
5-
// CHECK: Recursive use of 't0_v0'
6-
t0_v0 = t0_v0 + 1
75

86
t1_v1 = 1
97
t1_v1 = 2
@@ -17,12 +15,3 @@ t2_s0:
1715
// CHECK: invalid reassignment of non-absolute variable 't3_s0'
1816
t3_s0 = 1
1917

20-
21-
// CHECK: Recursive use of 't4_s2'
22-
t4_s0 = t4_s1
23-
t4_s1 = t4_s2
24-
t4_s2 = t4_s0
25-
26-
// CHECK: Recursive use of 't5_s1'
27-
t5_s0 = t5_s1 + 1
28-
t5_s1 = t5_s0

‎llvm/test/MC/ELF/weakref.s

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,4 +105,10 @@ alias:
105105
.weakref alias2, target
106106
.set alias2, 1
107107

108+
.weakref cycle0, cycle1
109+
.weakref cycle1, cycle0
110+
call cycle0
111+
# ERR: <unknown>:0: error: cyclic dependency detected for symbol 'cycle0'
112+
# ERR: [[#@LINE-2]]:1: error: expected relocatable expression
113+
108114
.endif

‎llvm/test/MC/Mips/set-sym-recursive.s

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
1-
# RUN: not llvm-mc -triple mips-unknown-linux %s 2>&1 | FileCheck %s
1+
# RUN: not llvm-mc -filetype=obj -triple=mips64 %s -o /dev/null 2>&1 | FileCheck %s --implicit-check-not=error:
22

3+
.set B, B + 1
4+
5+
# CHECK: :[[@LINE+1]]:11: error: cyclic dependency detected for symbol 'A'
36
.set A, A + 1
4-
# CHECK: :[[@LINE-1]]:9: error: Recursive use of 'A'
7+
# CHECK: :[[@LINE+1]]:7: error: expected relocatable expression
8+
.word A
59
.word A

0 commit comments

Comments
 (0)