[PATCH] D69411: [MC] Calculate difference of symbols in two fragments when possible

[Jian Cai via Phabricator via llvm-commits]

jcai19 added inline comments.


================
Comment at: llvm/lib/MC/MCExpr.cpp:568
+    // for interworking.
+    if (Asm->isThumbFunc(&SA))
+      Addend |= 1;
----------------
nickdesaulniers wrote:
> This patch adds checks for thumb, but the test case doesn't use a thumb target triple. Consider adding tests that exercise the newly added code.
It's actually checking if SA is flagged with .thumb_func, although adding .thumb_func right before label 9997 does not make the check become true? 


================
Comment at: llvm/lib/MC/MCExpr.cpp:573
+    // correct offset in .gcc_except_table
+    if (Asm->getBackend().isMicroMips(&SA))
+      Addend |= 1;
----------------
nickdesaulniers wrote:
> MicroMips?
It seems micromips has been used through out the file, will keep it for consistency.


================
Comment at: llvm/test/MC/AsmParser/directive_if_with_dot_symbol.s:1
+@ RUN: llvm-mc -filetype=obj -triple armv7a-linux-gnueabihf %s -o -
+
----------------
nickdesaulniers wrote:
> Is it possible to add tests for other ISA's, too?
I have not figured out an example with other ISA. The reason is MCFragment is transparent to assembly so I do not know an easy way to force two adjacent labels to be assigned to two MCFragment in the assembly file. For the same reason, the ISA extension right after this line is needed for the integrated assembler to place 9997 and the .if directive into two different fragments in the same section. 


================
Comment at: llvm/test/MC/AsmParser/directive_if_with_dot_symbol.s:6
+9997: nop ; .if . - 9997b == 2 ; nop ; .endif ;
+// CHECK-MESSAGES: :[[@LINE-1]]:17: error: expected absolute expression
----------------
nickdesaulniers wrote:
> We're checking that an error occurs? Wouldn't a test for this new feature test that an error does not occur? Or am I misunderstanding the test?
Yes, you are correct. I think I uploaded an incomplete version. Updated it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69411