[PATCH] D107133: [AVR] emit `MCSA_Global` references to `__do_global_ctors` and `__do_global_dtors`

[Matt Jacobson via Phabricator via llvm-commits]

mhjacobson added inline comments.


================
Comment at: llvm/lib/Target/AVR/AVRAsmPrinter.cpp:64
   const MCRegisterInfo &MRI;
+  bool EmittedStructorSymbolAttrs = false;
 };
----------------
benshi001 wrote:
> It would not be good to use such a flag. But would be better to put your code into MCTargetDesc/AVRTargetStreamer.cpp where `__do_copy_data` is emitted, and make a check like
> 
> ```
> if (current module has constructor)
>   emit do_global_ctors
> if (current module has destructor)
>   emit do_global_dtors
> ```
I'm not sure how to know, from `AVRTargetStreamer`, whether there are constructors/destructors.

If we don't want to keep `EmittedStructorSymbolAttrs`, then I suppose an alternative would be for `AVRAsmPrinter::emitXXStructor()` *always* to emit the global symbols.  It won't hurt anything for them to be emitted more than once -- it's just theoretically inefficient.

Can you explain more about your concern with `EmittedStructorSymbolAttrs`?  Is it bad to keep state inside `AVRAsmPrinter`?


================
Comment at: llvm/lib/Target/AVR/AVRAsmPrinter.cpp:206
+
+    MCSymbol *DtorsSym = OutContext.getOrCreateSymbol("__do_global_dtors");
+    DtorsSym->setExternal(true);
----------------
benshi001 wrote:
> Do we need to emit some comments for ` __do_global_dtors` as `__do_copy_data` has ?
Good idea -- I'll do that now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107133