[PATCH] D51464: clang: fix MIPS/N32 triple and paths

[YunQiang Su via Phabricator via cfe-commits]

wzssyqa added a comment.

ohhh. make check-all is needed, instead of make check

> test/CodeGen/target-data.c

  is due to duplicate line `MIPS-64EL: target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-n32:64-S128"'

> test/Driver/mips-cs.cpp

  is due to this test use the hardcode path `/mips-linux-gnu/', so mipsel and mips64el also need
      BiarchTripleAliases.append(begin(MIPSTriples), end(MIPSTriples));
  although MIPSTriples should in the last order.



================
Comment at: lib/Driver/ToolChains/Arch/Mips.cpp:109
 
+  if (ABIName.empty() && (Triple.getEnvironment() == llvm::Triple::GNUABIN32))
+    ABIName = "n32";
----------------
atanasyan wrote:
> If target triple is mips-mti-linux-gnuabin32 the code above set (incorrectly) the `ABIName` to `n64` and this statement will be `false`.
as I know mips-mti-linux-gnuabin32 is never used, at least for gcc.
Should we allow it?


================
Comment at: lib/Driver/ToolChains/Arch/Mips.cpp:85
 
   if (ABIName.empty() &&
       (Triple.getVendor() == llvm::Triple::MipsTechnologies ||
----------------
atanasyan wrote:
> Is possible to rewrite this piece of code (lines 85-114) as follows?
> ```
>   if (ABIName.empty() && (Triple.getEnvironment() == llvm::Triple::GNUABIN32))
>     ABIName = "n32";
> 
>   if (ABIName.empty() &&
>       (Triple.getVendor() == llvm::Triple::MipsTechnologies ||
>        Triple.getVendor() == llvm::Triple::ImaginationTechnologies)) {
>     ABIName = llvm::StringSwitch<const char *>(CPUName)
>                   .Case("mips1", "o32")
>                   .Case("mips2", "o32")
>                   .Case("mips3", "n64")
>                   .Case("mips4", "n64")
>                   .Case("mips5", "n64")
>                   .Case("mips32", "o32")
>                   .Case("mips32r2", "o32")
>                   .Case("mips32r3", "o32")
>                   .Case("mips32r5", "o32")
>                   .Case("mips32r6", "o32")
>                   .Case("mips64", "n64")
>                   .Case("mips64r2", "n64")
>                   .Case("mips64r3", "n64")
>                   .Case("mips64r5", "n64")
>                   .Case("mips64r6", "n64")
>                   .Case("octeon", "n64")
>                   .Case("p5600", "o32")
>                   .Default("");
>   }
> ```
OK. I didn't do like this because I am not very full confident about whether there are some case that it is set to N32 even -gnuabin32 is used.


================
Comment at: lib/Driver/ToolChains/Gnu.cpp:2082
     BiarchTripleAliases.append(begin(MIPSELTriples), end(MIPSELTriples));
-    BiarchTripleAliases.append(begin(MIPSTriples), end(MIPSTriples));
+    BiarchLibDirs.append(begin(MIPSN32ELLibDirs), end(MIPSN32ELLibDirs));
+    BiarchTripleAliases.append(begin(MIPSN32ELTriples), end(MIPSN32ELTriples));
----------------
atanasyan wrote:
> Why do you remove `BiarchTripleAliases.append(begin(MIPSTriples), end(MIPSTriples));` in that case only? Is it intended?
ohhh, my fault. I should remove from mipsel also.


================
Comment at: lib/Driver/ToolChains/Linux.cpp:47
   bool IsAndroid = TargetTriple.isAndroid();
+  std::string MipsCpu = "", Mips64Abi = "gnuabi64";
+  if (TargetEnvironment == llvm::Triple::GNUABIN32)
----------------
atanasyan wrote:
> - Do you need `MipsCpu` variable?
> - Is it possible to use any lightweight type like `StringRef` for the `Mips64Abi`?
oh, MipsCpu is not used here, while used in D50850.
In that patch. we need different CPU names:

    mipsel vs mipsisa32r6el

etc. It is my fault to split the patches.


StringRef  is not OK as, the return value of getMultiarchTriple is std::string.


================
Comment at: lib/Driver/ToolChains/Linux.cpp:118
   case llvm::Triple::mips64:
     if (D.getVFS().exists(SysRoot + "/lib/mips64-linux-gnu"))
       return "mips64-linux-gnu";
----------------
atanasyan wrote:
> If a user has two toolchains installed into "/lib/mips64-linux-gnu" and "/lib/mips64-linux-gnuabin32", this code always selects mips64-linux-gnu even if N32 ABI is requested. Is it intended?
Yes. It is intended.

I don't want my patch change the behavior of llvm/clang:
    the previous of llvm/clang behavior is perfering /lib/mips64-linux-gnu.

And on Debian, /lib/mips64-linux-gnu should never exists, we use
    /lib/mips64-linux-gnuabi64
and 
   /lib/mips64-linux-gnuabin32



================
Comment at: lib/Driver/ToolChains/Linux.cpp:118
   case llvm::Triple::mips64:
     if (D.getVFS().exists(SysRoot + "/lib/mips64-linux-gnu"))
       return "mips64-linux-gnu";
----------------
wzssyqa wrote:
> atanasyan wrote:
> > If a user has two toolchains installed into "/lib/mips64-linux-gnu" and "/lib/mips64-linux-gnuabin32", this code always selects mips64-linux-gnu even if N32 ABI is requested. Is it intended?
> Yes. It is intended.
> 
> I don't want my patch change the behavior of llvm/clang:
>     the previous of llvm/clang behavior is perfering /lib/mips64-linux-gnu.
> 
> And on Debian, /lib/mips64-linux-gnu should never exists, we use
>     /lib/mips64-linux-gnuabi64
> and 
>    /lib/mips64-linux-gnuabin32
> 
If there is a SDK, which is using `/lib/mips64-linux-gnu', and for Debian we use `/lib/mips64-linux-gnuabi64':

 there must at least one cannot work. So here, I choose to make sure the exists SDK working and leave the risk to Debian's official toolchain.
as the `/lib/mips64-linux-gnu' is previous than my patch by time.

By FHS and Debian's practice, cross toolchains should never be put under /lib directly, these manually installed software should exist in /usr/local or /opt. 



https://reviews.llvm.org/D51464