This is an archive of the discontinued LLVM Phabricator instance.

[OHOS] Add support for OpenHarmony
ClosedPublic

Authored by kpdev42 on Nov 17 2022, 5:27 AM.

Details

Summary

Add support for OpenHarmony OS

General OpenHarmony OS discussion on discourse thread "[RFC] Add support for OpenHarmony OS"
https://discourse.llvm.org/t/rfc-add-support-for-openharmony-os/66656

Diff Detail

Event Timeline

kpdev42 created this revision.Nov 17 2022, 5:27 AM
kpdev42 requested review of this revision.Nov 17 2022, 5:27 AM
kpdev42 updated this revision to Diff 476198.Nov 17 2022, 11:48 AM
kpdev42 edited the summary of this revision. (Show Details)
kpdev42 removed a reviewer: chandlerc.

Remove tls-related changes
Add link to dicourse thread

Ping) Any thoughts, questions?

xen0n added a subscriber: xen0n.Dec 6 2022, 12:09 AM

Please include context with the diff (https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

(Phab doesn't have the full repo backing each diff so the context lets us see what the surrounding code is)

arsenm added a subscriber: arsenm.Jan 26 2023, 8:44 AM

I'm generally of the opinion that anything and everything can go into Triple

llvm/include/llvm/ADT/Triple.h
742

As someone who's never heard of this operating system, this helper name isn't particularly enlightening

arsenm added inline comments.Jan 26 2023, 8:48 AM
llvm/test/Transforms/SafeStack/AArch64/unreachable.ll
3

New tests should use opaque pointers (has this one somehow missed conversion?)

jrtc27 added inline comments.Jan 26 2023, 8:55 AM
llvm/include/llvm/ADT/Triple.h
250

Keep blank line

270

Keep blank line

742

What if I give aarch64-liteos-gnu?

llvm/include/llvm/BinaryFormat/MinidumpConstants.def
118

What's with the 0x8206 gap?

llvm/test/Transforms/SafeStack/AArch64/abi.ll
3

These diffs don't belong here, the tests pass without them

DavidSpickett added inline comments.
llvm/test/Transforms/SafeStack/AArch64/abi.ll
3

This should have a comment if there is some OpenHarmony specific behaviour here.

llvm/test/Transforms/SafeStack/AArch64/unreachable.ll
3

Same here please note why OpenHarmony needs this specific check. Also note that FileCheck %s --check-prefix=OHOSMUSL will only check that prefix, not that plus the CHECK. I don't know if that is what you intended.

llvm/unittests/ADT/TripleTest.cpp
813

Is the "canonical" triple here arm-unknown-liteos-ohos ? I guess you are testing here to make sure that liteos implies ohos if ohos is not in the triple already.

kpdev42 updated this revision to Diff 495068.Feb 6 2023, 3:26 AM

Rebase and clean

kpdev42 marked an inline comment as done.Feb 6 2023, 3:39 AM
kpdev42 added inline comments.
llvm/include/llvm/ADT/Triple.h
742

We currently assume environment for liteos is always ohos. Thus gnu env will be ignored.

DavidSpickett added inline comments.Feb 27 2023, 2:48 AM
llvm/include/llvm/BinaryFormat/MinidumpConstants.def
118

Is this still relevant?

kpdev42 marked an inline comment as done.Feb 27 2023, 3:03 AM
kpdev42 added inline comments.
llvm/include/llvm/BinaryFormat/MinidumpConstants.def
118

Actually, no, it was changed from 0x8207 in initial diff to 0x8206 in current one

kpdev42 marked 2 inline comments as done.Feb 27 2023, 3:03 AM
DavidSpickett accepted this revision.Feb 27 2023, 3:12 AM

LGTM, all comments have been addressed.

Please add [llvm] at the start of the commit title as well. If you are landing it using arc, edit the title here in the web interface first. It has a habit of choosing that over the local version in my experience.

This revision is now accepted and ready to land.Feb 27 2023, 3:12 AM
This revision was landed with ongoing or failed builds.Feb 27 2023, 6:17 AM
This revision was automatically updated to reflect the committed changes.

LGTM, all comments have been addressed.

Please add [llvm] at the start of the commit title as well. If you are landing it using arc, edit the title here in the web interface first. It has a habit of choosing that over the local version in my experience.

Sorry, forgot to add this tag, mea culpa. Will add it in next patches. Thank you for the review