MCObjectStreamer: assign MCSymbols in the dummy fragment to offset 0.

[jyknight]

Summary

In MCObjectStreamer, when there is no current fragment, initially
symbols are created in a "pending" state and assigned to a dummy
empty fragment.

Previously, they were not being assigned an offset, and thus
evaluateAbsolute would fail if trying to evaluate an expression 'a -
b', where both 'a' and 'b' were in this pending state.

Fixes: https://llvm.org/PR41825

[peter.smith]

This looks reasonable to me. The one concern I had was that Checking over the case where getRelaxAll() is true, it looks like each instruction is created in its own fragment, then merged with the previous, triggering a call to flushPendingLabels which will set the offset correctly before the temporary offset of 0 could be misused. Might be worth a test case for the combination of .if and --mc-relax-all.

llvm/lib/MC/MCObjectStreamer.cpp

261

For consistency is it worth setting the offset to 0 here as well? From what I can tell this is only used for outputting Arm/Thumb mapping symbols and these really shouldn't be used for any kind of symbolic lookup, but it may be possible other uses for this function will be found in the future.

[jyknight]

Update for comment.

llvm/lib/MC/MCObjectStreamer.cpp

261

Agreed.

This function's semantics seem kinda broken, since sometimes it sets offset and sometimes doesn't. (shouldn't matter for the only current usage). So, refactored a bit, and done.

[nickdesaulniers]

llvm/test/MC/AsmParser/assembler-expressions.s

36

This is testing that an error is emitted? Don't we want to test that an error is *not* emitted?

[jcai19]

Thanks for this patch!

llvm/test/MC/AsmParser/assembler-expressions.s

36

I got a feeling this test case has something to do with my patch at https://reviews.llvm.org/D69411 :). I accidentally removed a nop instruction in the test case used to reproduce the issue in the said patch. Will update my test case. Thanks @jyknight for catching it.

[jyknight]

llvm/test/MC/AsmParser/assembler-expressions.s

36

No, that error is emitted when attempting to write textual assembly output (the first RUN line in the test), from parsing the textual input. In that case, we cannot evaluate differences between symbols, since we don't track any instruction sizes or anything, but that is OK -- it's irrelevant in real-world use-cases.

The other two RUN lines, where we emit an object file, succeed.

[MaskRay]

In MCObjectStreamer, when there is no current fragment, initially symbols are created in a "pending" state and assigned to a dummy empty fragment.

Maybe clarify that "pending" means SymContentsUnset.

The following snippet (contained in assembler-expressions.s) fails before this change.

.text
text1:
# In MCExpr.cpp, AttemptToFoldSymbolOffsetDifference cannot fold because MCValue(.).SymA is SymContentsUnset (and also MCValue(text1).SymB)
.if . - text1 == 0
	nop
.endif

The second .if does not emit an error.

.text
text1:
.if . - text1 == 0
	nop
.endif

text2:
        nop
# This does not error, because MCObjectStreamer.cpp EmitLabel changes the MCValue(.).SymA to SymContentsOffset.
.if . - text2 == 1
	nop
.else
	ret
.endif

Probably wait for a sign-off from other reviewers.

[peter.smith]

I'm happy with this too. I checked the ARM Mapping Symbol code, and it only uses the EmitLabelAtPos in one specific circumstance, a pending data mapping symbol which always has a MCDataFragment associated with it.

[nickdesaulniers]

Thanks, I tested the first two cases in https://bugs.llvm.org/show_bug.cgi?id=41825. I also did not see any more patterns of EmitLabel() followed by setOffset() in the tree. @peter.smith provides an excellent summary in https://reviews.llvm.org/D69411#1742030.

llvm/lib/MC/MCObjectStreamer.cpp

void MCObjectStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc) {
   // Otherwise queue the label and set its fragment pointer when we emit the
   // next fragment.
   auto *F = dyn_cast_or_null<MCDataFragment>(getCurrentFragment());
   if (F && !(getAssembler().isBundlingEnabled() &&
              getAssembler().getRelaxAll())) {
     Symbol->setFragment(F);
     Symbol->setOffset(F->getContents().size());
   } else {
+    // Assign all pending labels to offset 0 within the dummy "pending"
+    // fragment. (They will all be reassigned to a real fragment in
+    // flushPendingLabels())
+    Symbol->setOffset(0);
     PendingLabels.push_back(Symbol);
   }
 }
 
 void MCObjectStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc, MCFragment *F) {
   MCStreamer::EmitLabel(Symbol, Loc);
   getAssembler().registerSymbol(*Symbol);
   auto *DF = dyn_cast_or_null<MCDataFragment>(F);
   if (DF)
     Symbol->setFragment(F);
   else
     PendingLabels.push_back(Symbol);
 }
 
 void MCObjectStreamer::EmitULEB128Value(const MCExpr *Value) {
   int64_t IntValue;
   if (Value->evaluateAsAbsolute(IntValue, getAssemblerPtr())) {
     EmitULEB128IntValue(IntValue);
     return;
   }

llvm/test/MC/AsmParser/assembler-expressions.s

 # RUN: not llvm-mc -triple x86_64-unknown-unknown %s 2>&1 | FileCheck %s --check-prefix=ASM-ERR
 # RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown %s | llvm-objdump -j .data  -s - | FileCheck %s --check-prefix=OBJDATA
 # RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown %s | llvm-objdump -j .text -s - | FileCheck %s --check-prefix=OBJTEXT
 .data
 
 # OBJDATA: Contents of section .data
 # OBJDATA-NEXT: 0000 aa0506ff
-  
 foo2:
 # ASM-ERR: [[@LINE+1]]:5: error: expected absolute expression
 .if . - foo2 == 0
     .byte 0xaa
 .else
     .byte 0x00
 .endif
 
 foo3:
     .byte 5
 # ASM-ERR: [[@LINE+1]]:5: error: expected absolute expression
 .if . - foo3 == 1
    .byte 6
 .else
    .byte 7
 .endif
 
 .byte 0xff
 
 # nop is a fixed size instruction so this should pass.
-  
 # OBJTEXT: Contents of section .text
-# OBJTEXT-NEXT: 0000 9090ff34 25000000 00909090 78563412
-# OBJTEXT-NEXT: 0010 78563412 90
+# OBJTEXT-NEXT: 0000 909090ff 34250000 00009090 90785634
+# OBJTEXT-NEXT: 0010 12785634 1290
 
 .text
-  
 text1:
+# ASM-ERR: [[@LINE+1]]:5: error: expected absolute expression
+.if . - text1 == 0
+  nop
+.endif
+
+text2:
         nop
 # ASM-ERR: [[@LINE+1]]:5: error: expected absolute expression
-.if . - text1 == 1
+.if . - text2 == 1
   nop
 .else
   ret
 .endif
   push gs
   nop
   nop
   nop
 # No additional errors.
-# 
+#
 # ASM-ERR-NOT: {{[0-9]+}}:{{[0-9]+}}: error:
 
-  
-
-text2:
-  .long 0x12345678
 text3:
-  .fill (text3-text2)/4, 4, 0x12345678
+  .long 0x12345678
+text4:
+  .fill (text4-text3)/4, 4, 0x12345678
   nop