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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
