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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay 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;
----------------
jyknight wrote:
> 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.
I mean I cannot construct a test case to exercise the "Res.getSymB()" condition.

Thanks for spotting the  lld/test/ELF/x86-64-relax-got-abs.s issue. I'll special case `Res.isAbsolute` and add a test for `abs at gotpcrelx... abs = 42`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88625