[PATCH] D158587: [sanitizer][msan] VarArgHelper for loongarch64

[Limin Zhang via Phabricator via llvm-commits]

Ami-zhang added a comment.

In D158587#4639972 <https://reviews.llvm.org/D158587#4639972>, @xen0n wrote:

> I'm not looking at this in more detail right now (it's 3 am here), but isn't the LoongArch psABI mostly resembling RISCV, and not Mips?

I have read varargs part in the RISCV psABI  at https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc#va_list-va_start-and-va_arg and in Mips "elf64-2.4.pdf".  That of LoongArch psABI is indeed very similar to that of RISCV, and also similar to MIPS.

RISCV is not yet supported in the MSan vararg instrumentation, so i referred to the implementation based on Mips here. After testing and verifying,  the Mips-based vararg implementation in LoongArch works fine.

The ABI similarities between architectures could be coincidental, therefore we can't ignore the potential risks of sharing code.  I should pay attention and update it accordingly if there are any changes in the future.



================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:5840
+struct VarArgLoongArch64Helper : public VarArgHelper {
+  Function &F;
+  MemorySanitizer &MS;
----------------
vitalybuka wrote:
> Ami-zhang wrote:
> > vitalybuka wrote:
> > > It looks like exact clone of Mips64 helper.
> > > Only `      if (TargetTriple.getArch() == Triple::mips64) {` is missing
> > > 
> > > Why not just use Mips64 then?
> > Yes, the `VarArg` convention  in LoongArch's ABI is similar to that of Mips,  so `VarArgLoongArch64Helper` is here based on Mips.
> > 
> > If use Mips64 helper, like this:
> > 
> > ```
> > +  else if (TargetTriple.isMIPS64() || TargetTriple.isLoongArch64())
> >      return new VarArgMIPS64Helper(Func, Msan, Visitor);
> > ```
> > `VarArgMIPS64Helper` is a MIPS64-specific implementation. Adding `loongarch64` here, does it seem somewhat ambiguous, suggesting `LoongArch` as a variant of `MIPS `architecture? I don't know if it's good to use it directly like mentioned above. So is it okay like above or have any better suggestions?
> > 
> > Thanks for your time.
> > 
> How about something line this:
> 
> // Loongarch64 is not a MIPS, but the current vargs calling convention matches the MIPS.
> using VarArgLoongArch64Helper = VarArgMIPS64Helper;
> 
> Another alternative is to rename VarArgMIPS64Helper into something more general, maybe there is a name for this calling convention already?
Thanks for your suggestions. I'm inclined towards the first alternative. 

As for renaming `VarArgMIPS64Helper`,  we did consider renaming it to something more generic, but we haven't found a better name yet, and it's worth taking some time to research and find a suitable name. So for now, I will go with the first option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158587