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

[Peter Smith via Phabricator via llvm-commits]

psmith added reviewers: ostannard, eli.friedman.
psmith added a comment.

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.



================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:591
+  bool EmitSymbolAttribute(MCSymbol *S, MCSymbolAttr Attribute) {
+    bool val = MCELFStreamer::EmitSymbolAttribute(S, Attribute);
+
----------------
peter.smith wrote:
> Yes, to both of the above. This will need to be
> 
> ```
> bool EmitSymbolAttribute(MCSymbol *S, MCSymbolAttr Attribute) override
> ```
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.


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:590
 
+  bool EmitSymbolAttribute(MCSymbol *Symbol, MCSymbolAttr Attribute) override {
+    bool val = MCELFStreamer::EmitSymbolAttribute(Symbol, Attribute);
----------------
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.
```


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

https://reviews.llvm.org/D74927