This is an archive of the discontinued LLVM Phabricator instance.

[LLVM][OHOS] Clang toolchain and targets
ClosedPublic

Authored by kpdev42 on Mar 3 2023, 3:30 AM.

Diff Detail

Event Timeline

kpdev42 created this revision.Mar 3 2023, 3:30 AM
kpdev42 requested review of this revision.Mar 3 2023, 3:30 AM

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

clang/lib/Driver/ToolChains/CommonArgs.cpp
1404

Collapse this into if isohos && triple.getarch is....

1694

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.

clang/lib/Driver/ToolChains/Gnu.cpp
645–647

And musl is what OHOS uses? Please add that to the comment if so "for musl, which OHOS uses...".

2319

No riscv related changes needed in this file?

clang/lib/Driver/ToolChains/OHOS.cpp
165

Keep FIXMEs that refer to stuff outside the project downstream please :)

373
clang/test/Driver/ohos.c
241

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.

kpdev42 marked an inline comment as done.Mar 6 2023, 10:51 PM
kpdev42 added inline comments.
clang/test/Driver/ohos.c
241

Yes, musl implements pthread api, so we do not need to link with libpthread

kpdev42 updated this revision to Diff 504632.Mar 13 2023, 6:55 AM
kpdev42 marked an inline comment as done.

Address review comments and rebase

kpdev42 marked 6 inline comments as done.Mar 13 2023, 6:59 AM
kpdev42 updated this revision to Diff 504647.Mar 13 2023, 7:25 AM

Fix test case

DavidSpickett accepted this revision.Mar 13 2023, 7:57 AM

Looks good in general. If there are some incorrect details you'll find them later.

This revision is now accepted and ready to land.Mar 13 2023, 7:57 AM
This revision was landed with ongoing or failed builds.Mar 14 2023, 2:25 AM
This revision was automatically updated to reflect the committed changes.

Good morning,

I believe the two tests added in this patch, clang::ohos.c and clang::ohos.cpp are failing on the following buildbot:

https://lab.llvm.org/buildbot/#/builders/216/builds/18323

is anyone able to take a look?

thanks,
Tom W

Also failing on our Windows on Arm bot: https://lab.llvm.org/buildbot/#/builders/65/builds/8607

I think some lines need to account for Windows slashes, {{/|\\\\}} should do that.

It looks like these tests don't pass when LLVM_ENABLE_LINKER_BUILD_ID is set; I've issued a revert until this can be fixed to unblock the Fuchsia toolchain.

This revision is now accepted and ready to land.Mar 14 2023, 1:48 PM
This revision was automatically updated to reflect the committed changes.

Also failing on our Windows on Arm bot: https://lab.llvm.org/buildbot/#/builders/65/builds/8607

I think some lines need to account for Windows slashes, {{/|\\\\}} should do that.

Yes, sorry... Fixed https://github.com/llvm/llvm-project/commit/28997feb0c3ac243cb5cc89d7682993e23463ca7

MaskRay added inline comments.Jul 5 2023, 1:53 PM
clang/lib/Driver/ToolChains/OHOS.h
39

Why is useRelaxRelocations false?

kpdev42 marked an inline comment as done.Jul 13 2023, 2:09 AM
kpdev42 added inline comments.
clang/lib/Driver/ToolChains/OHOS.h
39

Thank you for mentioning. I would say that there is no precise reason for it. And it will be changed to true in the nearest future.