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

[Stefan Agner via Phabricator via llvm-commits]

falstaff84 marked 2 inline comments as done.
falstaff84 added inline comments.


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:590
 
+  bool EmitSymbolAttribute(MCSymbol *S, MCSymbolAttr Attribute) {
+    bool val = MCELFStreamer::EmitSymbolAttribute(S, Attribute);
----------------
MaskRay wrote:
> nickdesaulniers wrote:
> > If it's overriding a virtual method, please mark it `override`.
> I am very sure this is `emitSymbolAttribute` (and the reason I got blamed)
Not sure I understand, there is no definition of `emitSymbolAttribute` anywhere.


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:593
+
+    if (!IsThumb)
+      return val;
----------------
peter.smith wrote:
> 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.
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 :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74927