Comments 72
Экзотические варианты есть, но каждый такой вариант должен быть в осознаным и вызывать вопрос — а нужно ли так делать, если это вызывает такие проблемы?
А главное — иногда ты пишешь скучно, а хак получается из-за ошибки. И вот тогда супер полезно обратить на него пристальное внимание.
Я могу ошибаться, прошу пояснить. В вашем последнем примере volatile относится к указателю или к массиву, на который передаётся указатель?
Up: Сам спросил, сам отвечу :) да, если бы volatile был после *, то относился бы к самой переменной. Так что пример корректен.
Это некорректно, нужно понимать, что вы делаете. Чтобы было легче и не путать указатель на константу с константным указателем могу посоветовать такой хинт (не для продакшена): объявляйте все типы через using. А потом поиграйтесь в IDE (например CLion) с заменой типов — сразу будет понятно что и как раскрывается.
Вообще меня ещё в техникуме сильно гоняли на первом курсе по этому поводу, чтобы выработать понимание. Но с появлением auto и развитием STL всё это стало ненужно.
2. Я понимаю, что это и как работает. Я не понимаю, что и как осыпается при оптимизациях компилятора.
Ссылки — зло. Используйте объединения.
Ну тут всё просто. Если утрировать, то в си пофиг на const в большинстве случаев. Это больше для программиста подсказки. За исключением ситуаций когда объект лежит в readonly памяти. Если вы в такое упрётесь, то скорее всего вы это уже будете знать :)
Про volatile. Если вы в теле метода не меняете значение обычной переменной, то компилятор может вычислить все выражения с ней заранее, например при вызове функции; а всё что в циклах уже игнорировать и подставлять значения. Когда же переменная не volatile то компилятор не будет делать таких допущений. Грубо говоря такой код:
void foo(int i){
I = 1;
While(I != 0){...}
}
Превратится в такой:
void foo(int i){
I = 1;
While(true){...}
}
А с точки зрения ассемблера, какой нибудь cmp заменится на безусловный goto. Что будет куда более дружелюбно для конвейера.
Вот чтобы запретить компилятору такие фокусы, используется volatile. Ну или если вы что-то низкоуровневое программируете и напрямую читаете память.
Вот чтобы запретить компилятору такие фокусы, используется volatile.Даже с -О3 такое упрощение не будет проведено? Не помню этот кусок стандарта.
За исключением ситуаций когда объект лежит в readonly памяти. Если вы в такое упрётесь, то скорее всего вы это уже будете знать :)Ну, обычно знаю :) По-моему, регистры иначе как константный указатель на волатильную переменную и не объяснить. Ещё константные массивы как член структуры очень удобно объявлять, когда играем в ООП.
Даже с O3, если переменная помечена как volatile. Для того он собственно и создавался. Представьте, что у вас где-то в памяти лежат данные с какой-либо железяки, которые туда попадают допустим через DMA. Без volatile было бы очень проблемно отслеживать их изменение.
habr.com/ru/company/pvs-studio/blog/523696/#comment_22191288
Если на проекте слишком много такого кода, то, боюсь, проблема не в анализаторе.
мне любопытно, но сам не могу полностью понять как такая штука работает.
Мне кажется, что вы под влиянием вполне понятных эмоций недооцениваете опасение разработчиков. А именно: в C++ нет простого и очевидного способа удостоверится в том, что у вызваной функции нет нежелательных побочных эффектов. Причем не обязательно эффектов, которые завязаны на многопоточность, чтение I/O портов или какой-то другой хардкорной специфики.
Вот простой пример, в котором все работает в однопоточном режиме:
class Registry {
int * ptr_{nullptr};
public:
void register_pointer(int * ptr) { ptr_ = ptr; }
void remove_pointer() { ptr_ = nullptr; }
void increment() { if(ptr_) ++(*ptr_); }
};
Registry registry;
void func_with_side_effects() {
registry.increment();
}
int demo() {
int a[3]{ 0, 1, 2 };
struct RegistryModificator {
RegistryModificator(int * ptr) {
registry.register_pointer(ptr);
}
~RegistryModificator() {
registry.remove_pointer();
}
} registry_modificator{a};
if(0 == a[0]) {
func_with_side_effects();
if(0 == a[0]) return -1;
}
return 0;
}
int main() {
return demo();
}После вызова func_with_side_effects повторная проверка имеет смысл. Но PVS-Studio скажет, что она избыточна.
И можно было бы кивнуть на то, что ложные срабатывания — это неизбежность. Но посмотрите на это со стороны того, кто будет сопровождать код.
Если это будет человек, который не знает тонкостей работы func_with_side_effects и не имеет возможности (желания) разобраться с этим вызовом, то он может просто поверить статическому анализатору и удалить "лишнюю" проверку. Что затем приведет к потере работоспособности участка кода.
И да, здесь нет аналогии с бензопилой и ломом. Вполне себе обычный код, без каких-либо хитрых трюков. Т.к. С++ является императивным языком, разрешающим различные побочные эффекты, к выдаче предупреждений о том, что какие-то проверки не имееют смысла, следует относится с большой осторожностью.
С другой стороны, такой пример попахивает не очень хорошо, и если анализатор подсвечивает этот code smell, в этом тоже есть определенная польза, кто-то может захотеть отрефакторить его более очевидным способом.
Потому что кто-то может удалить проверку, которая выглядит избыточной, и без подсказки анализатора.
потому что код этот воняет
И как же его переписать, чтобы не вонял?
Перепишите проверку if (0 == a[0]) так, чтобы компилятор не смог использовать оптимизацию sequential read/compare elimination
А почему вы решили, что компилятор здесь может использовать эту оптимизацию?
Если это для вас ожидаемое поведение — отлично, только вот для большинства пользователей статического анализатора это большой сюрприз, и сообщение об этой повторной проверке для них — это не ложное срабатывание, а указание на то, что осторожно, здесь в засаде сидит медведь, если вы его туда посадили сами — классно, но если нет — обратите внимание, он там.
как только при очередном рефакторинге перестанет
Как только перестанет, это уже будет другой код. И о другом коде можно будет вести другой разговор. Пока речь идет о вполне себе корректном случае, где анализатор вводит в заблуждение.
Можно оправдываться тем, что от false positive никто не застрахован. Вполне понятная позиция.
особенно в проектах, над которыми работает больше сотни людей каждый день, а баги в нем стоят миллионы настоящих долларов.
Звучит как "сперва добейся".
В моем мире не просто «с анализатором лучше», а давно уже «без анализатора полный конец обеда», если у вас не так — я реально рад.
Я наивно полагаю, что целью маркетинговой программы PVS-Studio является продвижение своего продукта не только в нишу mission-critical, где анализаторы — это и так уже устоявшаяся практика, но и в нишу обычного говнокодинга, где нет ни бюджетов в сотни миллионов долларов, ни возможностей нанимать выпускников MTI, ни проектных команд в 100500 человек.
Так что можно сделать замечательный дефектоскоп, который отыскивает микротрещены в карбоновых корпусах болидов Формулы-1, а механики формулических команд будут с удовольствием рассказывать насколько этот дефектоскоп облегичил им жизнь (и все это будет чистой правдой)… Но ссылки на опыт Формулы-1 будут мало чего значить для владельца автомастерской в условном Урюпинске.
А PVS-Studio допускает бесплатное использование для некоммерческих проектов. И если некий разработчик возьмёт на свою совесть скормить анализатору кусочек коммерческого кода, в котором подозревает подвох, КМК никто не будет в обиде.
Тут речь не о том, нужен ли автомастерской в условном Урюпинске дефектоскоп или нет (скорее всего нужен). А о том, подойдет ли опыт механика из Формулы-1 в реалиях урюпинской автомастерской. Ведь одно дело обнаружить микротрещину на носовом обтекателе болида Формулы-1, где эта микротрещина может реально привести к разрушению в условиях гонки. И другое дело обнаружить микротрещину на капоте Лады Калина, с которой эта самая Лада Калина пробегает еще лет 10.
Ну и как следствие: в mission-critical софте false-posivite от анализатора может восприниматься как благо, как указание на потенциальную угрозу жизни пользователей софта. Тогда как в провинциальном говнокодинге аналогичный false-posivite — это отвлечение внимания разработка и удлинение сроков разработки (соответственно, увеличение стоимости).
Правда же сермяжная состоит в том, что чем раньше ошибку настоящую удалось заметить, тем дешевле ее починить, и в Урюпинске это не чуть ни менее верно, чем в Зеленограде, Берлине, Тель-Авиве, или Сан-Франциско.
Анализатор в существующий CI/CD-пайплайн добавляется без существенных затрат, и сам он стоит далеко не как самолет (а с разработчиками еще и на скидку можно договориться гораздо чаще, чем нет), и при этом он снижает нагрузку на все последующие этапы производства ПО, потому что ловит тупые ошибки программирования вроде копи-пасты неудачной прямо на излете, еще до попадания их в репозиторий или основную ветку.
Если действительно учли сроки и бюджет, изучили количество ложных срабатываний на своем коде, и выяснили, что для вас лично анализатор не нужен или даже вреден — честь вам и хвала, снимаю шляпу.
Уже объяснил, потому что повторная проверка этого условия сама по себе не меняет состояние абстрактной машины, которой оперирует стандарт. Пока ваша func_with_side_effects() трогает a[] — все скорее всего будет работать
А вы можете определиться с тем "скорее всего будет работать" или все-таки "сама по себе не меняет состояние"?
Вы понимаете выражение «сама по себе»?
Я не понимаю причинно-следственных связей между двумя вашими предложениями, а именно:
Уже объяснил, потому что повторная проверка этого условия сама по себе не меняет состояние абстрактной машины, которой оперирует стандарт.
Звучит как будто вы пишете о том, что вы понимаете. Т.е. здесь вы в себе уверены.
Пока ваша func_with_side_effects() трогает a[] — все скорее всего будет работать
А здесь уже сомнения: "скорее всего будет". Т.е. здесь уже не уверены.
Как из уверенности проистекает неуверенность мне не понятно.
Поэтому было бы интересно узнать: приведенный код корректен и будет работать (просто потому, что корректен) или же он может не работаеть, т.к. некорректен?
Приведенный код корректен и будет работать как задумано пока побочный эффект функции func_with_side_effects() приводит к невозможности применения компилятором sequential read/compare elimination.
Т.е. по факту:
- код корректен;
- анализатор ошибся.
Вот этот инвариант, и подобные ему, чаще всего существует только в голове у автора кода, и только пока он не забыл его.
Чаще всего такой код приходится писать потому, что лучшего решения силами существующей команды, в условиях существующих сроков и бюджетов, просто не представлялось возможным найти.
Принципиальная проблема этого кода в том, что в нем есть побочные эффекты, которые не видны в локальном контексте. И принципиальное решение было бы в устранении самих этих побочных эффектов. Но никак не то, что вы предложили:
например положите его внутрь no-inline-функции с побочными эффектами, таким же образом, как вы уже сделали с func_with_side_effects()
Т.к. вы предложили способ удовлетворить анализатор, а не способ предолеть изначальную хрупкость кода (хрупкость проистекает из-за использования функций с побочными эффектов).
И тут ж хотелось бы определится: мы решаем проблему кода (что может быть крайне сложно, т.к. есть ограничения команды, сроков, бюджетов) или же мы удовлетворяем анализатор.
С моей точки зрения анализатор здесь себя повел неадекватно. У него была информация о том, что неконстантная ссылка на локальные данные утекает куда-то во внешний контекст, а внутри функции вызваются внешние функции с хз какими побочными эффектами. И в таких условиях говорить разработчику "чувак, у тебя здесь ошибка" вместо "чувак, а ты точно уверен, что здесь нужна проверка, т.к. локальных модификаций я здесь не вижу" — это (как по мне) введение разработчика в заблуждение.
Так что в эту игру можно играть и вдвоем: разработчики анализаторов рассказывают на примерах как анализаторы отлавливают ошибки, а пользователи анализаторов на других примерах рассказывают, как анализатор берет на себя слишком много.
И в таких условиях говорить разработчику «чувак, у тебя здесь ошибка» вместо «чувак, а ты точно уверен, что здесь нужна проверка, т.к. локальных модификаций я здесь не вижу» — это (как по мне) введение разработчика в заблуждение.Мне кажется, мы с вами принципиально по разному подходим к интерпретации сообщений анализатора. Для меня они всегда указывают не на реальные ошибки, а на потенциальные, и все равно человек в итоге принимает решение, ошибка это или нет. Уже писал, что для меня ложные срабатывания анализатора — это уже сигнал, что с кодом что-то не так, и стоило бы посмотреть на него внимательнее, и возможно улучшить так, чтобы у анализатора не было претензий, но и блокировать релиз для всего этого я тоже не стану.
Кажется вы зря придираетесь. Анализатор кода будет всегда выдавать ложно положительные срабатывания, даже если у него есть вся-вся информация, смотрите теорему Райса, например. Если вам не нравятся false positive — не пользуйтесь анализаторами, они без них не могут. Если не нравиться конкретно этот — лучше напишите баг репорт.
«Анализатор ошибся», «анализатор вводит разработчиков в заблуждение», «анализатор тут неадекватен», «анализатор мог, но не сделал» — не способствует конструктивной дискуссии.
memset may be optimized away (under the as-if rules) if the object modified by this function is not accessed again for the rest of its lifetime (e.g. gcc bug 8537). For that reason, this function cannot be used to scrub memory (e.g. to fill an array that stored a password with zeroes). This optimization is prohibited for memset_s: it is guaranteed to perform the memory write. Third-party solutions for that include FreeBSD explicit_bzero or Microsoft SecureZeroMemory.Незнание as-if rule ведет к огромному количеству реальных уязвимостей в коде на С. Если вы все еще пишите критический для безопасность код С, уважайте это правило, пожалуйста.
Не использовать глобальную переменную.
Да, но нет.
class Registry {
int * ptr_{nullptr};
public:
void register_pointer(int * ptr) { ptr_ = ptr; }
void remove_pointer() { ptr_ = nullptr; }
void increment() { if(ptr_) ++(*ptr_); }
};
void func_with_side_effects(Registry & registry) {
registry.increment();
}
int demo() {
Registry registry;
int a[3]{ 0, 1, 2 };
struct RegistryModificator {
Registry & registry_;
RegistryModificator(Registry & registry, int * ptr)
: registry_{registry}
{
registry_.register_pointer(ptr);
}
~RegistryModificator() {
registry_.remove_pointer();
}
} registry_modificator{registry, a};
if(0 == a[0]) {
func_with_side_effects(registry);
if(0 == a[0]) return -1;
}
return 0;
}
int main() {
return demo();
}Никаких глобальных переменных. Тоже самое сообщение об ошибке.
Перед тем, как его сюда запостить, я проверил его на их on-line демонстраторе. Может, конечно, делал что-то не так, но диагностику от PVS-Studio получил.
А смысл его проверять, это старый код, который уже скорее всего не используется. А если и да, предъявить ошибки всё равно некому (у Андрея может быть другое мнение)
Если речь именно о коде ядра, то может там и не сильно всё меняется, но маловероятно, что в коде ядра прямо так уж много багов, т.к. на этом коде основано всё остальное, включая весь прикладной софт, и ошибки бы выявились раньше (если это прямо чистой воды ошибка). Хуже если это не ошибка, а какая-то неоптимальность, но найдет ли ее анализатор?
С МС вполне станется баг не фиксить, потому что другой софт использует его как недокоментированную фичу и не работает без этого бага. Вроде даже они сами это рассказывали
Они рассказывали, что у них есть слой совместимости, который может имитировать поведение (возможно, косячное) определенных системных вызовов для определенных версий программ, если данные версии некорректно работают при исправленном поведении системного вызова.
Когда для приложения включен режим совместимости, учитывается встроенная в винду база со списком старых версий программ и какие правки к каждой программе применять.
Немного ссылок по теме:
https://techcommunity.microsoft.com/t5/ask-the-performance-team/demystifying-shims-or-using-the-app-compat-toolkit-to-make-your/ba-p/374947
https://www.itnews.com.au/news/windows-compatibility-mode-resurfaces-old-flaws-473058
https://haacked.com/archive/2014/05/27/backwards-compatibility/
Например:
void SetSynchronizeVar(int * z){(*z)++;}
int main()
{
int flag = 0;
SetSynchronizeVar(&flag);
int X, Y = 1;
if (flag == 0)
{
X = Y;
if (flag == 0)
return 1;
}
return 2;
}11:1: error: V547 Expression 'flag == 0' is always true на вторую проверку.Вот хороший пример того, что data-flow-анализ у компилятора оказался лучше, чем у анализатора, и вот это предупреждение — действительно ложное срабатывание.
Зря вы вторую часть написали, щас начнется по новой (ну, вон, уже началось).
Сам я хоть PVS-Studio и не пользуюсь (пишу на C/C++ настолько редко и настолько мало, что в этом нет смысла), но возможностями статических анализаторов, встроенных в IDE семейства IntelliJ IDEA, пользуюсь постоянно, на разных языках. Ложные срабатывания бывают, да. Но в подавляющем большинстве случаев ложное срабатывание заставляет переписать код более по-человечески, так, что не только анализатору, но и человеку код понятнее. А если это не тот случай — ну, что ж, замьютить — это хоткей ткнуть (и багрепорт написать, по-хорошему. Многое правят).
Сугубо субъективно. Заранее извините.
Думать об анализаторах как о средстве поиска ошибок мне кажется не правильно, скорее это средство поиска потенциально проблемных мест.
Когда анализатор работает он говорит: "Чувак, я думаю тут лажа — проверь", но он не говорит "Чувак, тут ошибка — исправь". Поэтому анализаторы должны работать с паре с программистом, который знает о системе больше анализатора и сможет принять решение. Может там вообще умудрились подпаяться к транзисторам внутри проца и в любой момент поменять состояние системы (фантазия), анализатор не может знать об этом — для этого нужен программист.
С DMA пример был вообще из фантастики. Что планируется ожидать при чтении из буфера, с которым в текущий момент работает DMA? Судя по примеру проверчется элемент, в который DMA еще не записал данные. Для подобных проверок существуют спец. флаги в регистрах, которые нужно смотреть, но не значения в буфере.
Такой подход контрпродуктивен. Неидеальный инструмент вполне может быть полезен, а его использование экономически целесообразным.
Согласен с этим утверждением. Но и, признаться честно, технический евангелизм как практика у меня тоже особых симпатий не вызывает.
#include <stdio.h>
#include <inttypes.h>
#define FIFO_BITS 8
#define FIFO_SIZE (1<<FIFO_BITS)
#define FIFO_MASK (FIFO_SIZE-1)
typedef struct{
uint8_t st, en;
uint8_t buf[FIFO_SIZE-1];
uint8_t some_data;
}fifo_t;
void fifo_reset(fifo_t *f){
f->st = 0; f->en = 0; f->some_data = 100;
}
unsigned int fifo_size(fifo_t *f){
return (f->st - f->en) & FIFO_MASK;
}
void fifo_push(fifo_t *f, uint8_t data){
if(fifo_size(f)==1)return;
f->buf[f->en] = data;
f->en++;
f->en &= FIFO_MASK;
}
int16_t fifo_pop(fifo_t *f){
if(fifo_size(f)==0)return -1;
uint8_t tmp;
tmp = f->buf[f->st];
f->st++;
f->st &= FIFO_MASK;
return (int16_t)tmp;
}
int main(){
fifo_t fifo;
fifo_reset(&fifo);
for(int i=0; i<10; i++)fifo_push(&fifo, i);
for(int i=0; i<10; i++)fifo_pop(&fifo);
for(int i=0; i<1000; i++){
fifo_push(&fifo, i);
}
for(int i=0; i<10000; i++){
int16_t res = fifo_pop(&fifo);
printf("%i: %i (%i)\n", i, (int)res, fifo.some_data);
if(res < 0)break;
}
}Если что, ошибка не в форматах printf (данный пример я воспроизводил чуть ли не по памяти), а в выходе за пределы массива.
Задолбали со своим нытьем. Если людей не устраивает отсутствие гарантий со стороны анализатора (который к тому же скорее всего ещё и платный), значит проблема не в людях.
Не нужно учить хабаровчан как правильно любить ваш продукт. Пожалуйста, воздержитесь от написания третей статьи.
Автор примите непрошенных совет — не нужно писать таких статей. Выглядит как попытка оправдаться или попытка кого-то переубедить, но haters gonna hate. Вместо разбора одного синтетического примера и привязки статьи к треду с критикой я бы рассказал о том, что анализаторы это сложно, что C++ это сложно. Вывод о том, что разбираемый пример это мелочь и время на него тратить не надо оставил бы для читателей. Так вы и критику сможете учесть, уменьшите вероятность повторения такого непродуктивного разговора и дистанцируетесь от контекста, где к вам придираются по мелочам.
char get();
int foo(volatile char *p, bool arg)
{
if (p[1] == 1)
{
if (arg)
p[0] = get();
if (p[1] == 1) // OK
return 1;
}
// ....
return 3;
}
Неопределенный порядок выполнения
#pragma pack(push,1)
typedef union
{
struct eeprom_tag
{
signed short D0;
signed short D1;
unsigned char CRC;
};
unsigned char EepromAlignment[((sizeof(struct eeprom_tag)+2)/4)*4];
}
eeprom_t;
#pragma pack(pop)
...
eeprom_t Ep;
...
Ep.CRC=GetCRC((unsigned char*)&Ep, sizeof(struct eeprom_tag)-sizeof(Ep.CRC) );
Откуда предупреждение? В аргументах функции побочных эффектов нет, а sizeof вообще вычисляется на этапе компиляции.
Как писать чтобы предупреждения не было?
Продолжение: обидно за мнения про статические анализаторы кода