[PATCH] D26412: [XRay] Support AArch64 in LLVM

[Serge Rogatch via llvm-commits]

rSerge marked an inline comment as done.
rSerge added inline comments.


================
Comment at: lib/Target/AArch64/AArch64AsmPrinter.cpp:159
+      "xray_instr_map", ELF::SHT_PROGBITS,
+      ELF::SHF_ALLOC | ELF::SHF_GROUP | ELF::SHF_MERGE, 0,
+      CurrentFnSym->getName());
----------------
dberris wrote:
> rSerge wrote:
> > dberris wrote:
> > > I distinctly remember having to do something special for non-COMDAT sections, especially when doing section-per-function in x86. I suspect you'd want to replicate that logic here somehow, with a `//TODO` to merge the logic for ELF XRay sleds at a higher level for better re-use.
> > Yes, I see it now. Shall this code be CPU-independent? If so, let me look if I can refactor it in this patch...
> > The CPU-independent code seems to be
> > 
> > ```
> >   if (Sleds.empty())
> >     return;
> >   if (Subtarget->isTargetELF()) {
> >     auto PrevSection = OutStreamer->getCurrentSectionOnly();
> >     auto Fn = MF->getFunction();
> >     MCSection *Section = nullptr;
> >     if (Fn->hasComdat()) {
> >       Section = OutContext.getELFSection("xray_instr_map", ELF::SHT_PROGBITS,
> >                                          ELF::SHF_ALLOC | ELF::SHF_GROUP, 0,
> >                                          Fn->getComdat()->getName());
> >     } else {
> >       Section = OutContext.getELFSection("xray_instr_map", ELF::SHT_PROGBITS,
> >                                          ELF::SHF_ALLOC);
> >     }
> > 
> >     // Before we switch over, we force a reference to a label inside the
> >     // xray_instr_map section. Since EmitXRayTable() is always called just
> >     // before the function's end, we assume that this is happening after the
> >     // last return instruction.
> >     //
> >     // We then align the reference to 16 byte boundaries, which we determined
> >     // experimentally to be beneficial to avoid causing decoder stalls.
> >     MCSymbol *Tmp = OutContext.createTempSymbol("xray_synthetic_", true);
> >     OutStreamer->EmitCodeAlignment(16);
> >     OutStreamer->EmitSymbolValue(Tmp, 8, false);
> >     OutStreamer->SwitchSection(Section);
> >     OutStreamer->EmitLabel(Tmp);
> >     for (const auto &Sled : Sleds) {
> >          // CPU-dependent code block here (depends of 64- or 32- bit word width)
> >     }
> >     OutStreamer->SwitchSection(PrevSection);
> >   }
> >   Sleds.clear();
> > }
> > ```
> Yep, I think you found it. Would be happy to wait for the refactoring to make this simpler moving forward when we cover more architectures.
My team has suggested to defer the refactorring to a separate patch, so to keep the current patch AArch64-only. That refactoring would affect multiple CPUs, but the changes would have in common that they are all for ELF.
So I've uploaded a new version with code duplication and a TODO for the refactoring.


https://reviews.llvm.org/D26412