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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I'd like to see some more test cases for the error cases. Off the top of my head I can think of these cases:

  // Align fragment in between MCDataFragments
  9997: nop ;
        .align 4
        nop
  .if . - 9997b == 4 ;

  // Fill (I think) fragment in between MCDataFragments
  9997: nop ;
        .space 4
        nop
  .if . - 9997b == 4 ;

  // Relaxable (in Thumb) fragment in between MCDataFragments
  9997: nop ;
        b external
        nop
  .if . - 9997b == 4 ;

  // Constant pool between MCDataFragments,
  9997:
        ldr r0,=0x12345678 ;
        .ltorg
        nop  
  .if . - 9997b == 4 ;

There is also something called a MCCompactEncodingInstFragment that would cause this to fail, but I think that only exists as part of NaCl bundle locking, which I think isn't too much of a concern.



================
Comment at: llvm/lib/MC/MCExpr.cpp:565
 
-  if ((&SecA != &SecB) && !Addrs)
+   if (SecB.getFragmentList().getNextNode(*FragB) == FragA &&
+       isa<MCDataFragment>(FragA) && isa<MCDataFragment>(FragB)) {
----------------
I think it would be useful to add a comment explaining this line and what kind of expressions it is expected to support. It isn't intuitive given 
how AttemptToFoldSymbolOffsetDifference is called.




================
Comment at: llvm/test/MC/AsmParser/directive_if_offset.s:1
+@ RUN: clang -target arm-linux-gnueabihf %s -c -o /dev/null 2>&1 | FileCheck --allow-empty --check-prefix=CHECK-ARM %s
+@ RUN: clang -target thumb-linux-gnueabihf %s -c -o /dev/null 2>&1 | FileCheck --allow-empty --check-prefix=CHECK-THUMB %s
----------------
Be careful with module dependencies and targets.
- An llvm test cannot depend on clang, only tools that are in the llvm repository such as llvm-mc.
- All backends like ARM are optional so tests need to be guarded either by directory or by REQUIRES to prevent failures when backends aren't available. Looking at the directories lit.local.cfg:
```
if not 'X86' in config.root.targets:
    config.unsupported = True
```
Which is backed up by all of the tests in the directory using X86 backend triples. I think that this test if it remains in this location will need to be rewritten in x86. If the test needs Arm I think it ought to go in the MC/ARM directory.


================
Comment at: llvm/test/MC/AsmParser/directive_if_offset.s:1
+@ RUN: clang -target arm-linux-gnueabihf %s -c -o /dev/null 2>&1 | FileCheck --allow-empty --check-prefix=CHECK-ARM %s
+@ RUN: clang -target thumb-linux-gnueabihf %s -c -o /dev/null 2>&1 | FileCheck --allow-empty --check-prefix=CHECK-THUMB %s
----------------
peter.smith wrote:
> Be careful with module dependencies and targets.
> - An llvm test cannot depend on clang, only tools that are in the llvm repository such as llvm-mc.
> - All backends like ARM are optional so tests need to be guarded either by directory or by REQUIRES to prevent failures when backends aren't available. Looking at the directories lit.local.cfg:
> ```
> if not 'X86' in config.root.targets:
>     config.unsupported = True
> ```
> Which is backed up by all of the tests in the directory using X86 backend triples. I think that this test if it remains in this location will need to be rewritten in x86. If the test needs Arm I think it ought to go in the MC/ARM directory.
The test fails when there is asm output from llvm-mc.

```
llvm-mc -filetype=obj -triple=armv7a-linux-gnueabihf directive_if_offset.s -o /dev/null
echo $?
0
```
However when using the assembler output, this fails:
```
llvm-mc --triple armv7a-linux-gnueabihf directive_if_offset.s -filetype=asm -o /dev/null
directive_if_offset.s:7:5: error: expected absolute expression
.if . - 9997b == 4 ;
```
It may be that there is no way to resolve this in the asm output as the data structures might not exist. It will be worth a check to see though. I think that differences between the asm and obj outputs of llvm-mv are frowned on, but I don't think that they are necessarily fatal if coming from a assembler file and not something generated by clang. This might be a problem if the .if exists in inline assembler, not that I'd recommend anyone do that.


================
Comment at: llvm/test/MC/AsmParser/directive_if_offset.s:5
+nop
+.arch_extension sec
+9997: nop ;
----------------
Is this line significant for the test? If it isn't then it is worth taking it out.


================
Comment at: llvm/test/MC/AsmParser/directive_if_offset.s:8
+.if . - 9997b == 4 ;
+// CHECK-ARM-NOT: :[[@LINE-1]]:5: error: expected absolute expression
+// CHECK-THUMB-NOT: :[[@LINE-2]]:5: error: expected absolute expression
----------------
Unless the location is particular significant I'd recommend just "error: expected absolute expression". This will make it a bit less sensitive if the change is added to.


================
Comment at: llvm/test/MC/AsmParser/directive_if_offset.s:10
+// CHECK-THUMB-NOT: :[[@LINE-2]]:5: error: expected absolute expression
+orr r1, r1, #1 ;
+.else ; orr r1, r1, #2;
----------------
You could use llvm-objdump -d on the ELF output to test that the correct instruction had been generated. Normally we'd use llvm-mc -filetype=asm and use FileCheck on that but that isn't working in this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69411