[PATCH] D88625: MCExpr::evaluateAsRelocatableImpl : allow evaluation of non-VK_None MCSymbolRefExpr when MCAsmLayout is available

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: llvm/lib/MC/MCExpr.cpp:816
+          if (Res.getRefKind() != MCSymbolRefExpr::VK_None || !Res.getSymA() ||
+              Res.getSymB() || Res.getConstant())
+            return false;
----------------
MaskRay wrote:
> I cannot test `Res.getSymB` in the conditions because (1) `foo - bar` can result into an early error (2) `foo + foo` will cause a `report_fatal_error` crash.
Are you saying that the code, as it is now, has this problem? (If so, I don't understand what you mean).

But, looking at the test failure shown for lld/test/ELF/x86-64-relax-got-abs.s shows that "return false" was wrong here -- we shouldn't be _failing_ to evaluate, rather, we should be returning the symbol ref expr as-is. (`Res = MCValue::get(SRE, nullptr, 0); return true;` -- the fallthrough case below.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88625/new/

https://reviews.llvm.org/D88625