gost-engine
Avoid unaligned data access for grasshopper cipher.
Compiler can produce incorrect instructions for arch with strict memory
access like ARM. Root case is compiler assuming some data type is always
aligned properly.
Pointer cast from unaligned pointer to this type `grasshopper_w128_t`
can trigger segmentation fault.
Кстати, по поводу остальных мест - скорее всего надо просто убрать ALIGN(16), чтоб компилятор не думал, что там есть выравнивание.
Кстати, по поводу остальных мест - скорее всего надо просто убрать
ALIGN(16), чтоб компилятор не думал, что там есть выравнивание.
В общем, моё мнение такое — я не одобряю этого PR, так как он испортит, а не улучшит код.
- Лучше вообще не использовать align лишний раз (как и cast'ы к большему размеру) чем заниматься таким с выравниванием по 16 байт (которое ниоткуда логически не следует, и это уже магия (в плохом смысле), которую нужно помнить, а значит будущий код не защищен от таких "ошибок" (так как они не рациональны) — с моей точки зрения, это просто случайные "фиксы"). Не нужно сообщать компилятору ложную информацию (про align(16)) и он не сгенерирует unaligned код.
- Лучше фиксить реальные баги — где есть реальный segmentation fault — (пример с
charbuf_to_uintпоказывает, что тут это не так) — иначе, это просто нагромождение исправлений того что не сломано. Так же я рекомендую написать тесты, которые падают до фикса и работают после, доказывающие как наличие бага, так и эффективность фикса. - Такое нагромождение кода невозможно разевьювить.
Если принять этот PR, то и дальше и во всем коде надо будет выравнивать всё по 16 байт, такой тренд задан. Мы готовы и хотим это делать? Почему нигде люди таким не занимаются?
Если принять этот PR, то и дальше и во всем коде надо будет выравнивать всё по 16 байт, такой тренд задан. Мы готовы и хотим это делать? Почему нигде люди таким не занимаются?
Я повторюсь это недопонимание. Не нужно ничего явно выравнивать, это работа компилятора правильно размещать данные. Нужно не мешать компилятору несовместимыми кастами указателей.
Посмотрите как реализованы другие блочные шифры.
Кстати, по поводу остальных мест - скорее всего надо просто убрать
ALIGN(16), чтоб компилятор не думал, что там есть выравнивание.
Это так не работает. Выравнивание есть всегда, на основе типа данных и ABI. И компилятор ожидает этого выравнивания.
Например если функция получает int* компилятор уверен что указатель кратен четырём. Если это SSE тип то компилятор знает что он выровнен по 16 байтам, и использует инструкции для работы с выровненными данными.
ALIGN(16) добавлен потому что на x86 архитектуре SSE требует выровненных данных. На ARM архитектуре работа с int требует выровненных данных.
В общем, моё мнение такое — я не одобряю этого PR, так как он испортит, а не улучшит код.
Это не улучшение, а исправление реальных и возможных сбоев. Эти сбои никогда не воспроизведутся на x86 архитектуре.
1. Лучше вообще не использовать align лишний раз (как и cast'ы к большему размеру) чем заниматься таким с выравниванием по 16 байт (которое ниоткуда логически не следует, и это уже магия (в плохом смысле), которую нужно помнить, а значит будущий код не защищен от таких "ошибок" (так как они не рациональны) — с моей точки зрения, это просто случайные "фиксы"). Не нужно сообщать компилятору ложную информацию (про align(16)) и он не сгенерирует unaligned код.
Позвольте объяснить для чего используется ALIGN. Для переносимого кода. Если бы писалось для компьютеров с SSE то можно было бы просто использовать SSE тип данных. Но используются типы данных универсальные для всех архитектур, однако, чтобы с ними можно было работать как с SSE их нужно ДОПОЛНИТЕЛЬНО выровнить, чтобы был безопасный cast. Для архитектур без SSE это НЕНУЖНОЕ выравнивание, но оно безвредное.
Абсолютно согласен что касты к большему типу не нужны. Я расскажу как делал этот MR Добавил флаг компилятору показывающий все неправильные касты из меньшего типа к большему, и исправил предупреждения.
Компилятор всегда генерирует aligned код. Потому что так работает процессор.
2. Лучше фиксить реальные баги — где есть реальный segmentation fault — (пример с `charbuf_to_uint` показывает, что тут это не так) — иначе, это просто нагромождение исправлений того что не сломано. Так же я рекомендую написать тесты, которые падают до фикса и работают после, доказывающие как наличие бага, так и эффективность фикса.
charbuf_to_uint это исправление недопустимого каста. про тесты согласен, только проблема в том что для их запуска нужен arm.
3. Такое нагромождение кода невозможно разевьювить.
В каждом коммите менятся только одна функция, и не более 20 изменений строк. Подскажите какие требования к изменениям?
@shanechko, спасибо аз ответ. Извиняюсь, что не внимательно посмотрел ваш PR.
У меня есть доступ к разным архитектурам, так что если вы напишете тесты, которые воспроизводят эти проблемы мы, вероятно, сможем их запустить. Даже если это будет не часто — это возможность тестирования на будущее.
Ранее, мы специально делали тесты такими (есть даже отключение MIPS_FIXADE для mipsel), и гоняли их да других архитектурах, чтоб всплывали проблемы с alignment.
ps. В теории, есть возможность вернуть CI на ppc64le, s390x и arm64 на Travis-CI (там это всё ещё доступно для Open Source repositories).
7499536 2021-12-16 Fix align-cast for test_derive.c (Mikhail Labiuk)
[Теперь я понял что] тут всё ОК. -Wcast-align=strict вывел этот варнинг, который фиксится. (Как я понял, в этом PR фиксятся в основном/только варнинги, а не реальные segmentation faults на которые наткнулись. Это ОК. Но можно было это написать в commit message, Так как я этого сразу не понял.) В этом коммите поменяется вывод key "id" на разных архитектурах (в зависимости от endianness), но тут это и не важно, так как главное там показать в дебаге, что ключи одинаковые. Спасибо.
Как я понял, в этом PR фиксятся в основном/только варнинги, а не реальные segmentation faults на которые наткнулись.
В таком случае тесты (для этих багов) не нужны! Я изначально не понял, что это фиксы варнингов и говорит про тесты и вообще не правильно понимал что тут фиксится.
На ARM архитектуре работа с int требует выровненных данных.
Ранее код был протестирован на sparc64 м mipsel (c MIPS_FIXADE 0) где доступ к int реально требует выровненных данных. А так же на armv7hf. Те segmentation faults, которые возникали были исправлены.
Далее, если вопрос сводится к фиксу варнинга -Wcast-align. Этот варнинг не входит ни в -Wall ни даже в -Wextra. Если включить -Wcast-align то при сборке openssl этот варнинг возникает 175 раз. Почему не внести сначала в openssl исправление реальных и возможных сбоев?
ps. И, на сколько я понимаю, на ARM ядро линукс умет исправлять unaligned access (как у mipsel).