[PATCH] D145227: [LLVM][OHOS] Clang toolchain and targets

[David Spickett via Phabricator via llvm-commits]

DavidSpickett added a comment.

I'm not familiar with the details of toolchain config, but added some general comments.



================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1404
+  // OHOS-specific defaults for PIC/PIE
+  if (Triple.isOHOSFamily()) {
+    switch (Triple.getArch()) {
----------------
Collapse this into `if isohos && triple.getarch is...`.


================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1702
+  if (TC.getTriple().isOHOSFamily() && UNW != ToolChain::UNW_None) {
+    CmdArgs.push_back("-l:libunwind.a");
+    return;
----------------
Please add a comment explaining this. Even if it is just "OHOS is only compatible with libunwind". At least then we know it's nothing more mysterious.


================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:645
 
-      if (WantPthread && !isAndroid)
+      // We don't need libpthread neither for bionic nor for musl
+      if (WantPthread && !isAndroid && !isOHOSFamily)
----------------
And musl is what OHOS uses? Please add that to the comment if so "for musl, which OHOS uses...".


================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:2318
+      "mipsel-linux-gnu", "mips-img-linux-gnu", "mipsisa32r6el-linux-gnu",
+      "mipsel-linux-ohos"};
 
----------------
No riscv related changes needed in this file?


================
Comment at: clang/lib/Driver/ToolChains/OHOS.cpp:164
+  // For compatibility with arm-liteos sysroot
+  // FIXME: Remove this when we'll use arm-liteos sysroot produced by build.py.
+  addPathIfExists(
----------------
Keep FIXMEs that refer to stuff outside the project downstream please :)


================
Comment at: clang/lib/Driver/ToolChains/OHOS.cpp:372
+    // FIXME: gnu or both???
+    CmdArgs.push_back("--hash-style=both");
+  }
----------------
Maybe this helps? https://github.com/llvm/llvm-project/blob/e00c73c856a325008afead10cfb3e9d0fc4a1e41/clang/lib/Driver/ToolChains/Linux.cpp#L235

(no idea myself)


================
Comment at: clang/test/Driver/ohos.c:240
+
+// CHECK-OHOS-PTHREAD-NOT: -lpthread
+
----------------
This one is checking that we do not link to a pthread library, because when using musl, it already provides one?

Just checking you're not accepting the option but doing the opposite here. Worth adding a comment to explain the reasoning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145227