[MC][ARM] make Thumb function also if type attribute is set

[nickdesaulniers]

Summary

Make sure to set the bottom bit of the symbol even when the type
attribute of a label is set after the label.

GNU as sets the thumb state according to the thumb state of the label.
If a .type directive is placed after the label, set the symbol's thumb
state according to the thumb state of the .type directive. This matches
GNU as in most cases.

From: Stefan Agner <[e-mail]>

This fixes:
https://bugs.llvm.org/show_bug.cgi?id=44860
https://github.com/ClangBuiltLinux/linux/issues/866

[nickdesaulniers]

llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

590

If it's overriding a virtual method, please mark it override.

597

This cast<MCSymbolElf looks redundant.

[MaskRay]

llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

590

I am very sure this is emitSymbolAttribute (and the reason I got blamed)

[peter.smith]

I think this is close, but there are a couple of test failures with ninja check-llvm

llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

591

Yes, to both of the above. This will need to be

bool EmitSymbolAttribute(MCSymbol *S, MCSymbolAttr Attribute) override

593

Sadly I don't think that this is going to work as IsThumb is the state at the point emitSymbolAttribute(S, Attribute) is called, but not necessarily the state at the declaration of S. With this change I see a couple of test failures:

Failing Tests (2):
    LLVM :: MC/ARM/mixed-arm-thumb-bl-fixup.ll
    LLVM :: MC/ARM/thumb2-beq-fixup.s

An extract from one points out the problem:

  .code  16
  .thumb_func
thumb_caller:
  beq.w internal_arm_fn
  beq.w global_arm_fn
  beq.w global_thumb_fn
  beq.w internal_thumb_fn

  .type  internal_arm_fn,%function
  .code  32
internal_arm_fn:
  bx  lr

At the point .type internal_arm_fn, %function is called IsThumb is true, emitSymbolAttribute will call setIsThumbFunc which is then cached. When internal_arm_fn is defined the state is Arm but the cache of ThumbFuncs remains.

I think this particular case might be fixed by only calling setIsThumbFunc when S is a definition. When internal_arm_fn is defined IsThumb will be false so we'll be ok.

It won't fix something like:

.thumb
corner_case_thumb:
 bx lr
.arm
some_arm_function:
.type corner_case_thumb, %function

I think that this is much less likely in practice, and would require the ArmBackend to track IsThumb for each symbol whether it was a function or not.

[falstaff84]

llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

590

Not sure I understand, there is no definition of emitSymbolAttribute anywhere.

593

Hm, I see, I guess the only alternative is to actually store the Thumb information for every label. I was wondering whether we could change the semantics of ThumbFuncs in MCAssembler to contain all "potential" functions. My idea was to setIsThumbFunc whenever a label is emitted in a Thumb section, and then check where isThumbFunc is called whether that symbol was an actual function. But this seems not entirely trivial since isThumbFunc() is used in several places, and also in location where the ELF STT types are not available... So I gave up on this approach.

Your suggestion seems to pass all tests with the exception of the corner case you mentioned. However, it really seems to me that if such code exists, then the .type should be moved, also for readability :-)

[falstaff84]

• Check if Symbol has previously been defined
• Remove unnecessary cast
• Mark function as override

[psmith]

I think you'll need to rebase and change to emitSymbolAttribute. I also think we could do with a comment. Other that that I think this is looking good. Adding some more reviewers to see if there are any objections, or alternative suggestions. Personally I'm inclined to accept as we get the following case wrong in the linux kernel (https://github.com/torvalds/linux/blob/master/arch/arm/kernel/entry-common.S) which has an idiom:

label:
   ...
   ENDPROC(label)

Where the ENDPROC has the .type label, %function

I think that the only case where we'd have got the type right and this change would break is:

.arm
label:
  ...
.thumb
.type label, %function

I think that changing state mid-function and setting the type afterwards is sufficiently unlikely that it is worth the change. The alternative is to track the state for every label with emitLabel, then lookup that state when .type is encountered. Even then this isn't ideal as we could have:

label:
.thumb
...
.type label, %function

Which would imply tracking state per address.

llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

590

Can we add a comment explaining why we are overriding (extending) EmitSymbolAttribute? Something like:

// If a label is defined before the .type directive sets the label's type then the label can't be recorded as thumb function when the label is defined. We override emitSymbolAttribute() which is called as part of the parsing of .type so that if the symbol has already been defined we
can record the label as Thumb. FIXME: there is a corner case where the state is changed in between the label definition and the .type
directive, this is not expected to occur in practice and handling it would require the backend to track IsThumb for every label.

591

Apologies I typed the above wrong. it should be:

bool emitSymbolAttribute(MCSymbol *S, MCSymbolAttr Attribute) override

I think that this changed relatively recently. I think that if you rebased on top of master then you'd need to change this for LLVM to build.

[efriedma]

I did not realize implicit .thumb_func was a thing.

I'm worried we're going to fix this edge case, and introduce some other weird edge case. I would prefer that we infer the marking based on whether the first instruction in the function is Thumb, as opposed to the state of the assembler when some label/directive is written. And if first instruction isn't known to be ARM or Thumb, we should error. (We can figure this out the same way we generate mapping symbols.)

[psmith]

I think using .type symbol, %function is more common when a file has to assemble in both Arm or Thumb state depending on whether -marm or -mthumb is given to the assembler.

Some experiments with GNU as imply that it does:

• .thumb_func always results in a symbol with type STT_FUNC and bit 0 set to 1, regardless of the state at the point.
• .type sym, %function before definition of sym, results in a symbol with type STT_FUNC and bit 0 set if the state at the point of the definition of sym is Thumb, 1 if the state is Arm at the point
• .type sym, %function after the definition of sym will use the state of the first instruction to determine, initial data, such as .word is skipped over.
• If a symbol is set to Thumb ahead of definition, a .type sym, %function after the definition will not correct it.
  
  For the case where .type comes after the definition. I think that using mapping symbols could work well, although I don't think that they could be used directly as we only record the last mapping symbol and we need the first mapping symbol after the definition. I think that if the pending labels have been flushed then a reverse search up the Symbols defined in MCAssembler looking for the closest mapping symbol in the same fragment as the definition would work.

I think an error message could be a bit more complicated to implement, especially if the type is set at the point of definition as in effect we'd need to catch the next mapping symbol and check with the definition, there is more than one code-path to track down. Maybe a follow up patch.

Test code for GNU as.

  .syntax unified

  .section .text.1, "ax", %progbits
  .global sym      
  .thumb_func
sym:
  .arm
  bx lr      

  .section .text.2, "ax", %progbits        
  .thumb
  .global sym2
  .type sym2, %function
sym2:
  .arm
  bx lr

  .section .text.3, "ax", %progbits        
  .thumb
  .global sym3
sym3:
  bx lr
  .arm
  bx lr
  .type sym3, %function      

  .section .text.4, "ax", %progbits        
  .arm
  .global sym4
sym4:
  bx lr
  .thumb
  bx lr
  .type sym4, %function      
        
 .section .text.5, "ax", %progbits
 .thumb
sym5:
 bx lr
 .pushsection .text
 .arm
sym6:
 bx lr
 .type sym5, %function   
 .type sym6, %function
 .popsection .text
        
 .section .text.7, "ax", %progbits
 .arm
 .type sym7, %function       
sym7:
 .thumb
 bx lr

 .thumb
 .section .text.8, "ax", %progbits
 .thumb_func
sym8:
 .arm
 bx lr
 .type sym8, %function       

 .thumb
 .section .text.9, "ax", %progbits
 .type sym9, %function        
sym9:
 .arm
 bx lr
 .type sym9, %function

 .section .text.10, "ax", %progbits
 .arm
 .type sym10, %function
 .thumb       
sym10:
 .arm
 bx lr
 .type sym10, %function

 .section .text.11, "ax", %progbits
 .thumb

sym11:
 .word 0
 bx lr
 .type sym11, %function

Gives:

16: 00000001     0 FUNC    LOCAL  DEFAULT    8 sym5
18: 00000000     0 FUNC    LOCAL  DEFAULT    1 sym6
21: 00000000     0 FUNC    LOCAL  DEFAULT    9 sym7
24: 00000001     0 FUNC    LOCAL  DEFAULT   10 sym8
27: 00000001     0 FUNC    LOCAL  DEFAULT   11 sym9
30: 00000001     0 FUNC    LOCAL  DEFAULT   12 sym10
33: 00000001     0 FUNC    LOCAL  DEFAULT   13 sym11
37: 00000001     0 FUNC    GLOBAL DEFAULT    4 sym
38: 00000001     0 FUNC    GLOBAL DEFAULT    5 sym2
39: 00000001     0 FUNC    GLOBAL DEFAULT    6 sym3
40: 00000000     0 FUNC    GLOBAL DEFAULT    7 sym4

[MaskRay]

The rules seem complex and error-prone. Are these any cases we should emit a warning? Will be nice to communicate that with binutils people as well.

On the Linux kernel side, it'd be nice to make a change anyway if that moves us away from an edge case.

[psmith]

In D74927#1891751, @MaskRay wrote:

The rules seem complex and error-prone. Are these any cases we should emit a warning? Will be nice to communicate that with binutils people as well.

Strictly speaking I'd say that the only explicit contradiction is where .thumb_func is used and yet the state is actually ARM. When .type symbol, %function is used, the user is implicitly relying on the Assembler to set the type of the symbol appropriately. If the assembler knows enough to warn then it knows enough to set the type of the symbol correctly. This requires a bit more tracking of state than the current rather arbitrary set of heuristics that both GNU as and MC are using. Actual practical usage of state changes are small as most files tend to be entirely Arm or entirely Thumb so the simple heuristics work for 99% of the cases. In practice I suspect that using a similar set of heuristics in both GNU and LLVM will aid portability, this would mean warning if the assembler detected that the ARM/Thumb state of the symbol disagreed with the ARM/Thumb state of the mapping symbols.

[falstaff84]

In D74927#1891751, @MaskRay wrote:

[...]
On the Linux kernel side, it'd be nice to make a change anyway if that moves us away from an edge case.

I tried changing Linux first, but it is not entirely trivial as there are different cases (where only the ENDPROC macro is used and hence would require more invasive changes). Also I think the Linux case is quite straight forward, as it is always defined within the label and at a point where the assembler is still in Thumb mode. Hence my current approach using the mode when the type directive is found works...

In D74927#1891713, @psmith wrote:

[...]
For the case where .type comes after the definition. I think that using mapping symbols could work well, although I don't think that they could be used directly as we only record the last mapping symbol and we need the first mapping symbol after the definition. I think that if the pending labels have been flushed then a reverse search up the Symbols defined in MCAssembler looking for the closest mapping symbol in the same fragment as the definition would work.

From the comments I guess an approach using mapping symbol is requested? While I understand conceptually what we want I am a bit lost how to implement this exactly. From what I understand we generate mapping symbols of some kind already? I tried to find them in code but wasn't really successful... Can you maybe give me a more concrete pointer?

[psmith]

The mapping symbols $a, $t and $d are emitted whenever there is a state change. See the EmitMappingSymbol() functions in ARMELFStreamer.cpp . As an example:

.thumb
label:
bx lr
.arm
bx lr
.type label, %function

would result in an object file with:

label:
$t
bx lr
$a
bx lr

The last mapping symbol emitted is cached in LastEMSInfo, and the last mapping symbol for a section is cached in LastMappingSymbols for when ChangeSection() is called. IIUC this doesn't help us very much as LastEMSInfo will only give us the same information as IsThumb. For the example above at the point that the .type label, %function directive were encountered we'd have LastEMSInfo pointing to the $a and IsThumb = false. What we need to do is find the closest mapping symbol at or after label, the "$t" in this case. I can think of a couple of approaches:
1.) Set up a map from MCSymbol to EmitMappingSymbolInfo that records the first "$a" or "$t" mapping symbol seen after each Symbol, if we then encounter a .type symbol, %function we can look up the state.
2.) All symbols, including mapping symbols will be stored in MCAssembler::Symbols. These are accessible via symbol_begin(), symbol_end() and symbols(). In theory we could walk the symbols in reverse on encountering the .type directive, stopping when we encounter the label we are setting the .type for. There are some details that will need to be resolved; symbols, including mapping symbols that are pending, will need to be flushed. I'm making the assumption that symbols are added to MCAssembler::Symbols in source order, but this will need to be checked.

Apologies for all the details, there may be a simpler way of doing this.

[falstaff84]

In D74927#1892934, @psmith wrote:

[..]
Apologies for all the details, there may be a simpler way of doing this.

No worries, thanks a lot for the pointer that definitely will help. Will try to tackle this but will probably take some days.

[nickdesaulniers]

• rebase, add Peter's comment
• with D96632 (and hacking around https://github.com/ClangBuiltLinux/linux/issues/1286) I can build a THUMB2 defconfig Linux kernel.
• with this, I can boot it.

[nickdesaulniers]

• fix lint warning

[MaskRay]

Code LG. There are some test suggestions.

llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

605

Needs an ifunc test.

Defining ifunc is usually two lines

.type ifunc, %gnu_indirect_function
.set ifunc, resolver

For a STB_GLOBAL ifunc (common cases), add .globl ifunc

llvm/test/MC/ARM/thumb-function-address.s

14

After Name:, it is recommended to use CHECK-NEXT: to make sure the Value: matches the next line, not a faraway line which happens to have Value: 0x1.

CHECK:      Symbol {
CHECK:      Name: func_label
CHECK-NEXT:    Value: 0x1

21

Please add another case suggested by Peter:

@@ Note: GNU as sets the value to 1.
.thumb
label:
bx lr
.arm
bx lr
.type label, %function

The value is 0 (1 in GNU as)

[MaskRay]

llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

605

When there are multiple symbols, I usually prefer llvm-readelf -s which is much conciser.

I usually include the Value Size Type Bind Vis Ndx Name header line to let readers know the column meanings. You may omit some columns which are not useful (e.g. I often omit Num:)

[nickdesaulniers]

• convert test to llvm-readelf, add test for mixed .thumb/.arm, add ifunc test, use CHECK-NEXT

[nickdesaulniers]

llvm/test/MC/ARM/thumb-function-address.s

28–29

should this comment be bit 0?

[MaskRay]

llvm/test/MC/ARM/thumb-function-address.s

2

Add a file-level comment, something like:

@@ GNU as sets the thumb state according to the thumb state of the label.
@@ If a .type directive is placed after the label, set the symbol's thumb state according to the thumb state of the .type directive. This matches GNU as in most cases.

(Please reflow)

And consider adding this to the description of the patch as well. In the description, you can additionally mention that it is difficult to fully implement GNU as's rule, so we choose this heuristic.

28–29

Yes, bit 0.

30

Nit: align the columns (just to make it look prettier).

[nickdesaulniers]

• add file-level comment, fix bit 0/1 comments, align columns in test

[falstaff84]

Thanks @nickdesaulniers for bringing this to the finish line!

llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

case MCAF_Code32:
       return; // Change to ARM mode
     case MCAF_Code64:
       return;
     case MCAF_SubsectionsViaSymbols:
       return;
     }
   }
 
+  bool EmitSymbolAttribute(MCSymbol *S, MCSymbolAttr Attribute) {
+    bool val = MCELFStreamer::EmitSymbolAttribute(S, Attribute);
+
+    if (!IsThumb)
+      return val;
+
+    auto *Symbol = cast<MCSymbolELF>(S);
+    unsigned Type = cast<MCSymbolELF>(Symbol)->getType();
+    if (Type == ELF::STT_FUNC || Type == ELF::STT_GNU_IFUNC)
+      getAssembler().setIsThumbFunc(Symbol);
+
+    return val;
+  };
+
 private:
   enum ElfMappingSymbol {
     EMS_None,
     EMS_ARM,
     EMS_Thumb,
     EMS_Data
   };
 
   struct ElfMappingSymbolInfo {
     explicit ElfMappingSymbolInfo(SMLoc Loc, MCFragment *F, uint64_t O)

llvm/test/MC/ARM/thumb-function-address.s

+@ RUN: llvm-mc -filetype=obj -triple=armv7-linux-gnueabi %s -o %t
+@ RUN: llvm-readobj --symbols %t | FileCheck %s
+
+  .syntax unified
+  .text
+  .thumb
+func_label:
+  .type func_label, %function
+
+@ Check func_label address has bit 1 set.
+@ CHECK:   Symbol {
+@ CHECK:     Name: func_label (7)
+@ CHECK:     Value: 0x1
+@ CHECK:     Size: 0
+@ CHECK:     Binding: Local (0x0)
+@ CHECK:     Type: Function (0x2)
+@ CHECK:     Other: 0
+@ CHECK:     Section: .text (0x2)
+@ CHECK:   }
+