Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementation of JEP 391: macOS/AArch64 Port #29

Open
wants to merge 28 commits into
base: macos-aarch64
Choose a base branch
from

Conversation

KaperD
Copy link

@KaperD KaperD commented Dec 9, 2021

Это всё вместе. Можно билдить и запускать тесты)

@KaperD KaperD requested a review from AntonKozlov December 9, 2021 17:17
@AntonKozlov
Copy link
Member

О, круто. Я долго смотрел первый PR из серии -- если там бессмысленные запросы -- посмотрю этот, он может быть делает мои вопросы бесмысленными

@AntonKozlov
Copy link
Member

Здесь по прежнему не хватает коммита JDK-8253819: copy linux_aarch64 -> bsd_aarch64. Просто тяжело понять, что добавляется в новых файлах. Плюс, именно из-за этого приходится делать новые коммиты

JDK-8253819: remove functions for sve
JDK-8253819: return removed fuctions

-- то, что были добавлены новые функции в sve или были удалены функции, стало бы очевидно. Хочется проверить, что ничего не забыто.

Т.е. хочется, чтобы до JDK-8253819: Implement os/cpu for macOS/AArch64 коммита были вставлены два коммита: c3fc678 и предыдущий.

К ним есть вопрос: в bsd был добавлен файл, которого не было в linux

anton@mercury:~/proj/jdk15u-dev$ git show -C -C --stat 47dc6d23e
commit 47dc6d23e521de7356d46f8f1120944d3349d007
Author: Danil Bubnov <[email protected]>
Date:   Wed Dec 1 23:38:07 2021 +0500

    Copy files from linux_aarch64

 .../atomic_linux_aarch64.hpp => bsd_aarch64/atomic_bsd_aarch64.hpp}           |   0
 .../bytes_bsd_aarch64.inline.hpp}                                             |   0
 .../copy_linux_aarch64.inline.hpp => bsd_aarch64/copy_bsd_aarch64.inline.hpp} |   0
 .../{linux_aarch64/copy_linux_aarch64.s => bsd_aarch64/copy_bsd_aarch64.s}    |   0
 .../globals_linux_aarch64.hpp => bsd_aarch64/globals_bsd_aarch64.hpp}         |   0
 .../icache_linux_aarch64.hpp => bsd_aarch64/icache_bsd_aarch64.hpp}           |   0
 .../orderAccess_linux_aarch64.hpp => bsd_aarch64/orderAccess_bsd_aarch64.hpp} |   0
 .../{linux_aarch64/os_linux_aarch64.cpp => bsd_aarch64/os_bsd_aarch64.cpp}    |   0
 .../{linux_aarch64/os_linux_aarch64.hpp => bsd_aarch64/os_bsd_aarch64.hpp}    |   0
 .../prefetch_bsd_aarch64.inline.hpp}                                          |   0
 .../thread_linux_aarch64.cpp => bsd_aarch64/thread_bsd_aarch64.cpp}           |   0
 .../thread_linux_aarch64.hpp => bsd_aarch64/thread_bsd_aarch64.hpp}           |   0
 .../vmStructs_linux_aarch64.hpp => bsd_aarch64/vmStructs_bsd_aarch64.hpp}     |   0
 src/hotspot/os_cpu/bsd_aarch64/vm_version_bsd_aarch64.cpp                     | 126 ++++++++++++++++++++++++++++++++++++
 14 files changed, 126 insertions(+)

@AntonKozlov
Copy link
Member

Плюс, не хватает openjdk/jdk@0d0e9ba

Этот бекпорт же по-прежнему основан на
KaperD/jdk@5e4dae9 ?

Разделение на ... JDK-8253817: Support macOS Aarch64 ABI in Interpreter ... JDK-8254941: Implement Serviceability Agent for macOS/AArch64 для ревью не важно -- если не помогает, можно не делать. Но и не мешает.

@AntonKozlov
Copy link
Member

Но реально круто, что это почти полноценный порт! (чуть-чуть причесать историю коммитов и поревьюить ещё надо).

Мне нравится один патч, который можно тестить. Пойду попробую.

@KaperD
Copy link
Author

KaperD commented Dec 10, 2021

Плюс, не хватает openjdk/jdk@0d0e9ba

Он же не нужен нам, такого файла в jdk15 нет. В jdk15 я добавил изменение в os_bsd.cpp

Этот бекпорт же по-прежнему основан на
KaperD/jdk@5e4dae9 ?

Да

@AntonKozlov
Copy link
Member

Плюс, не хватает openjdk/jdk@0d0e9ba

Он же не нужен нам, такого файла в jdk15 нет. В jdk15 я добавил изменение в os_bsd.cpp

А, изменение есть, точно
2a542c9#diff-1f93205c2e57bee432f8fb7a0725ba1dfdbe5b901ac63010ea0b43922e34ac12. Я просто ожидал отдельный коммит по аналогии с 50c22d4.

Ок, здесь всё норм :)

GlebSolovev and others added 23 commits December 12, 2021 17:45
Backport ec9bee68660acd6abf0b4dd4023ae69514542256
Backport 36998b006d6a9ee8145004778534d1f35a55e068
Backport 2c8f4e202bd7e5b78edb32580b8a836660493d9c
@@ -139,7 +169,8 @@ void InterpreterRuntime::SignatureHandlerGenerator::pass_object() {
__ cbnz(temp(), L);
__ mov(r0, zr);
__ bind(L);
__ str(r0, Address(to(), next_stack_offset()));
static_assert(sizeof(jobject) == wordSize, "");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ой, вот тут нужно изменить на STATIC_ASSERT

Copy link
Member

@AntonKozlov AntonKozlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Почему-то отсутствует
openjdk/jdk@d178376 -- тест успешно проходит?

Comment on lines 1261 to 1263
// Enable WXWrite: the function is called by c1 stub as a runtime function
// (see another implementation above).
MACOS_AARCH64_ONLY(ThreadWXEnable wx(WXWrite, thread));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Этот hunk не нужен -- JRT_ENTRY уже содержит нужное переключение (в отличие от jdk17+)

kern_return_t kr;
kr = task_set_exception_ports(mach_task_self(),
EXC_MASK_BAD_ACCESS | EXC_MASK_ARITHMETIC,
EXC_MASK_BAD_ACCESS | EXC_MASK_ARITHMETIC
AARCH64_ONLY(| EXC_MASK_BAD_INSTRUCTION),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

выравнивание

@KaperD
Copy link
Author

KaperD commented Jan 13, 2022

Почему-то отсутствует openjdk/jdk@d178376 -- тест успешно проходит?

Этого теста нет в jdk15, его добавили позже openjdk/jdk@a6ab9e4#diff-b9d4d7f230fe35edab9920b7e6ce180e9dd5c26b567721414e6be7fa3a2329b6

@AntonKozlov
Copy link
Member

Тест действительно проходит, удивительно :) Выглядит замечательно! Мне хочется интегрировать именно этот PR, он примерно соответствует единоразовой интеграции реализации JEP.

Но ещё хочется немного поребейзить коммиты. Мне кажется, было ошибкой интегрировать #22, т.к. он есть составная часть этого PR. Проблема в том, что коммиты Глеба должны пойти именно до этого PR. Поэтому нужен ребейз и похоже force-update, чтобы правильно обработать #22. Например так, переработанная ветка macos-aarch64 с патчами из этого PR https://github.com/AntonKozlov/jdk15u-dev/tree/macos-aarch64-rebase

@KaperD
Copy link
Author

KaperD commented Jan 13, 2022

Тест действительно проходит, удивительно :) Выглядит замечательно! Мне хочется интегрировать именно этот PR, он примерно соответствует единоразовой интеграции реализации JEP.

Но ещё хочется немного поребейзить коммиты. Мне кажется, было ошибкой интегрировать #22, т.к. он есть составная часть этого PR. Проблема в том, что коммиты Глеба должны пойти именно до этого PR. Поэтому нужен ребейз и похоже force-update, чтобы правильно обработать #22. Например так, переработанная ветка macos-aarch64 с патчами из этого PR https://github.com/AntonKozlov/jdk15u-dev/tree/macos-aarch64-rebase

А небольшие исправления лучше добавить ребейзом в исходные коммиты или можно оставить отдельным коммитом?

@AntonKozlov
Copy link
Member

А небольшие исправления лучше добавить ребейзом в исходные коммиты или можно оставить отдельным коммитом?

Пока отдельным отлично. Большинство из этих коммитов, относящихся к Implementation JEP захочется слить попозже в один большой.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants