Вопрос по visual-c++, gcc, c++ – Насколько плохо «if (! This)» в функции-члене C ++?

73

Если я сталкиваюсь со старым кодом, который делаетif (!this) return; Насколько серьезный риск в приложении? Это опасная бомба замедленного действия, требующая немедленного поиска и уничтожения всего приложения, или это больше похоже на запах кода, который можно спокойно оставить на месте?

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

Представь себеCLookupThingy класс имеетневиртуальном CThingy *CLookupThingy::Lookup( name ) функция-член. Видимо, один из программистов в те ковбойские времена столкнулся с множеством сбоев, когда NULLCLookupThingy *s передавались из функций, и вместо того, чтобы исправлять сотни сайтов вызовов, он незаметно исправил Lookup ():

CThingy *CLookupThingy::Lookup( name ) 
{
   if (!this)
   {
      return NULL;
   }
   // else do the lookup code...
}

// now the above can be used like
CLookupThingy *GetLookup() 
{
  if (notReady()) return NULL;
  // else etc...
}

CThingy *pFoo = GetLookup()->Lookup( "foo" ); // will set pFoo to NULL without crashing

Я обнаружил этот драгоценный камень ранее на этой неделе, но теперь я не уверен, стоит ли его исправлять. Это основная библиотека, используемая всеми нашими приложениями. Некоторые из этих приложений уже были отправлены миллионам клиентов, и, похоже, они работают нормально; в этом коде нет сбоев или других ошибок. Удалениеif !this в функции поиска будет означать исправление тысяч сайтов вызовов, которые потенциально могут передавать NULL; неизбежно некоторые будут пропущены, введя новые ошибки, которые будут появляться случайно в течение следующегогод развития.

Поэтому я склонен оставить это в покое, если в этом нет крайней необходимости.

Учитывая, что это технически неопределенное поведение, насколько опасноif (!this) на практике? Стоит ли исправлять трудовые недели, или можно рассчитывать на MSVC и GCC для безопасного возвращения?

Наше приложение компилируется в MSVC и GCC и работает в Windows, Ubuntu и MacOS. Переносимость на другие платформы не имеет значения. Данная функция гарантированно никогда не будет виртуальной.

Редактировать: Вид объективного ответа, который я ищу, - что-то вроде

«Текущие версии MSVC и GCC используют ABI, где не виртуальные члены действительно статичны с неявным параметром« this »; поэтому они будут безопасно переходить в функцию, даже если« this »равно NULL» или«будущая версия GCC изменит ABI так, что даже не виртуальным функциям потребуется загрузка цели перехода из указателя класса» или«текущий GCC 4.5 имеет несовместимый ABI, где иногда он компилирует не виртуальные члены как прямые ветви с неявным параметром, а иногда как указатели на функции смещения класса».

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

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

Этот вопрос может показаться субъективным, а не конструктивным - ЭТО НЕТ! У него есть очень объективный и определенный ответ - «Убери, это бомба замедленного действия». Он также имеет несколько субъективных ответов, в зависимости от опыта пользователей со старыми устаревшими кодовыми базами ... Xeo
Это не редкость в реализациях классов очень низкого уровня. Думай струнный класс. В результате тщательного анализа отказов, где вы хотели бы, чтобы возникло исключение? Вам нравится брать вину в классе, который вы тщательно протестировали, и отвечать на телефонные звонки, когда он взрывается? Или вы делегируете сбой классу, который ближе к коду пользователя, что делает очевидным, что он выдвинул аргумент? Это, конечно, положительно влияет на практику, если она не генерирует никаких исключений - это злой кодекс. Hans Passant
Я думаю, что это принадлежитDailyWTF. DOK
Вызов NULL - неопределенное поведение в C ++.так что, вероятно, не очень хорошая идея, чтобы держать вокруг. Joachim Isaksson
Это довольно ужасно, но в вашей ситуации, боюсь, вы ничего не можете с этим поделать, учитывая огромную базу развертывания. Alex B

Ваш Ответ

11   ответов
2

Ты можешьсмело исправляйте это сегодня возвращая ошибочный объект поиска.

class CLookupThingy: public Interface {
  // ...
}

class CFailedLookupThingy: public Interface {
 public:
  CThingy* Lookup(string const& name) {
    return NULL;
  }
  operator bool() const { return false; }  // So that GetLookup() can be tested in a condition.
} failed_lookup;

Interface *GetLookup() {
  if (notReady())
    return &failed_lookup;
  // else etc...
}

Этот код все еще работает:

CThingy *pFoo = GetLookup()->Lookup( "foo" ); // will set pFoo to NULL without crashing
Проблема в том, что не существует только одной функции GetLookup (), которая возвращаетCLookupThingys. Они приходят (буквально) из тысячи различных источников, в том числе функций на противоположной стороне границы DLL. Я должен был бы исправить все такие места, чтобы вместо них возвращался тип failed_lookup, а это означает, что неизбежно некоторые будут пропущены и будут обнаружены ошибки. Crashworks
@Crashworks: я добавил отдельный ответ для этого случая. Neil G
@Crashworks: у вас есть много функций GetLookup? Или много абонентов GetLookup? Neil G
Много, много функций GetLookup (). (Или, точнее, много разных функций, которые возвращают потенциально NULL CLookupThingy *.) Crashworks
2

только если вы педантичны по поводу формулировки спецификации. Однако, несмотря на это, это ужасный, опрометчивый подход, потому что он скрывает программную ошибку. По этой причине я быУдалить это, даже если это означает значительную работу. Это не немедленный (или даже среднесрочный) риск, но это не очень хороший подход.

Такое поведение, скрывающее ошибки, на самом деле тоже не то, на что вы хотите положиться. Представьте, что вы полагаетесь на это поведение (т.е.не имеет значения, действительны ли мои объекты, это все равно будет работать!), а затем, по какой-то опасности, компилятор оптимизируетif утверждение в конкретном случае, потому что это может доказать, чтоthis не является нулевым указателем. Это законная оптимизация не только для какого-то гипотетического будущего компилятора, но и для очень реальных, современных компиляторов.
Но, конечно, поскольку ваша программа не является правильно сформированной, онапроисходит что в какой-то момент вы передаете это нульthis около 20 углов. Взрыв, ты мертв.
Это очень надумано, правда, и этого не произойдет, но вы не можете быть на 100% уверены, что это все ещене может возможно бывает.

Обратите внимание, что когда я выкрикиваю «удалить!», Это не значит, что их нужно удалить.немедленно или в одной крупной рабочей силе. Вы можете удалить эти проверки одну за другой, когда сталкиваетесь с ними (когда вы все равно что-то изменяете в одном и том же файле, избегаете перекомпиляций), или просто выполнять поиск по тексту (желательно в часто используемой функции) и удалить эту.

Поскольку вы используете GCC, вы можете быть__builtin_return_address, что может помочь вам удалить эти проверки без огромных трудовых ресурсов и полностью нарушить весь рабочий процесс и сделать приложение полностью непригодным для использования.
До снимите флажок, измените его, чтобы вывести адрес вызывающего абонента, иaddr2line скажет вам местоположение в вашем источнике. Таким образом, вы сможете быстро идентифицировать все местоположения в приложении, которые ведут себя неправильно, поэтому вы можете это исправить.

Так что вместо

if(!this) return 0;

измените одно местоположение за раз на что-то вроде:

if(!this) { __builtin_printf("!!! %p\n", __builtin_return_address(0)); return 0; }

Это позволяет идентифицировать недопустимые сайты вызовов для этого изменения, в то же время позволяя программе «работать как задумано» (при необходимости вы также можете запросить вызывающего абонента). Исправьте каждое плохое место, один за другим. Программа все равно будет "работать" как обычно.
Как только больше не будет адресов, удалите чек полностью. Возможно, вам все равно придется исправить один или другой сбой, если вам не повезло (потому что он не показывал, пока вы тестировали), но это должно случаться очень редко. В любом случае это должно помешать вашему коллеге кричать на вас.
Удалите один или два чека в неделю, и в конечном итоге ни один не останется. Между тем жизнь продолжается, и никто не замечает, что ты вообще делаешь.

TL; DR
Что касается «текущих версий GCC», вы можете использовать не виртуальные функции, но, конечно, никто не может сказать, что может сделать будущая версия. Однако я считаю крайне маловероятным, что в будущей версии ваш код будет поврежден. Не у некоторых существующих проектов есть такая проверка (я помню, что у нас было буквально сотни из них при завершении кода Code :: Blocks когда-то, не спрашивайте меня почему!). Производители компиляторов, вероятно, не хотят, чтобы десятки / сотни главных разработчиков проектов были недовольны намеренно, только чтобы доказать свою точку зрения.
Также рассмотрим последний абзац («с логической точки зрения»). Даже если эта проверка будет аварийно завершена и сгорит с будущим компилятором, она все равно будет аварийно завершена.

if(!this) return; утверждение несколько бесполезно, посколькуthis никогда не может быть нулевым указателем в правильно сформированной программе (это означает, что вы вызвали функцию-член для нулевого указателя). Это не значит, конечно, что это не могло произойти. Но в этом случае программа должна аварийно завершить работу или прервать работу с утверждением. Ни при каких условиях такая программа не должна продолжаться тихо.
С другой стороны, вполне возможно вызвать функцию-член наинвалид объект, который случаетсяненулевой, Проверяем лиthis нулевой указатель, очевидно, не улавливает этот случай, но это точно такой же UB. Таким образом, помимо скрытия неправильного поведения, эта проверка также выявляет только половину проблемных случаев.

Если вы идете по формулировке уточнения, используяthis (который включает проверку, является ли это нулевым указателем) - неопределенное поведение. Строго говоря, это «бомба замедленного действия». Однако я бы не сталразумно Считаем, что проблема как с практической, так и с логической точки зрения.

Изпрактическое точка зрения, это не имеет значения, действительно ли вычитать указатель, который недействителен, пока вы неразыменовать Это. Да, строго по письму это запрещено. Да, теоретически кто-то может создать процессор, который будет проверять недействительные указатели, когда вынагрузка их и вина. Увы, это не тот случай, если ты настоящий.Излогический точка зрения, предполагая, что проверкабудем взорвать, это все еще не произойдет. Для выполнения этого оператора должна быть вызвана функция-член, и (виртуальный или нет, встроенный или нет) использует недопустимыйthis, который он делает доступным внутри тела функции. Если одно незаконное использованиеthis взрывается, другой тоже. Таким образом, проверка устаревает, потому что программа уже аварийно завершает работу ранее.

n.b .: Эта проверка очень похожа на «безопасную идиому удаления», которая устанавливает указатель наnullptr после удаления (используя макрос или шаблонsafe_delete функция). Предположительно, это «безопасно», потому что не приводит к сбою, удаляя один и тот же указатель дважды. Некоторые люди даже добавляют избыточныйif(!ptr) delete ptr;.
Как вы знаете,operator delete гарантированно не работает с нулевым указателем. Что означает не больше и не меньше, чем, установив указатель на нулевой указатель, вы успешно устранили единственный шанс обнаружить двойное удаление (это программная ошибка, которую нужно исправить!). Он не является более «безопасным», но вместо этого скрывает некорректное поведение программы. Если вы удалите объект дважды, программадолжен тяжело врезаться.
Вы должны либо оставить удаленный указатель в покое, либо, если вы настаиваете на фальсификации, установить для него ненулевой недействительный указатель (такой как адрес специальной «недопустимой» глобальной переменной или магическое значение, подобное-1 если вы будете - но вы не должны пытатьсямошенничать и скрыть аварию, когда это произойдет).

20

Как и все неопределенное поведение

if (!this)
{
   return NULL;
}

этоявляется бомба ждет взрыва. Если это работает с вашими текущими компиляторами, вы вроде бы везучие, в некотором роде неудачники!

Следующая версия тех же компиляторов может быть более агрессивной и воспринимать это как мертвый код. Какthis никогда не может быть нулевым, код можно «безопасно» удалить.

Я думаю, что лучше, если вы удалите его!

@Ben С точки зрения процессора, вы правы. В соглашениях о вызовах x86 указатель this становится неявным первым параметром функции. На самом деле он не используется в качестве адреса в коде загрузки, пока вы не попытаетесь получить доступ к одному из членов класса. Это, конечно, все "неопределенное", что касается стандарта C ++; это деталь реализации x86 ABI. Crashworks
То есть Сам тест не UB, но он ничего не делает в отсутствие UB. То, что он делает в присутствии UB, конечно, не определено. MSalters
@Doug: Разыменование нулевого указателя любым способом - UB, и дляthis чтобы быть нулевым, нужно было бы вызвать функцию-член на нульCLookupThingy*, т.е.,if (!this) будет оцениваться как истинное, если вы уже находитесь в UB-стране. ildjarn
@Ben - Для вызова функции-члена вам нужен объект, членом которого является функция. Если указатель нулевой, естьявляется нет объекта. То, что x86 не всегда может обнаружить это, является одной из причин, по которой он не определен. Если бы все системы всегда выходили из строя (или нет), поведение было бы определено! Bo Persson
@ildjarn: у меня сложилось впечатление, что вызов функции-члена через указатель экземпляра не сразу разыменовывает этот указатель. Моя ментальная модель заключается в том, что указатель будет просто передан в качестве первого «скрытого» аргумента «простой» функции. Я ошибаюсь? Ben
5

создать функцию, не являющуюся членом

CThingy* SafeLookup(CLookupThing *lookupThing) {
  if (lookupThing == NULL) {
    return NULL;
  } else {
    return lookupThing->Lookup();
  }
}

Тогда должно быть достаточно легко найти каждый звонок наLookup функция-член и замените его безопасной функцией, не являющейся членом.

39

старомодной версииSafeNavigationOperator, Как вы говорите, в новом коде я бы не рекомендовал этого, но для существующего кода я бы оставил его в покое. Если вы в конечном итоге измените его, я бы позаботился о том, чтобы все обращения к нему были хорошо покрыты тестами.

Изменить, чтобы добавить: вымог выберите удалить его только в отладочных версиях вашего кода через:

CThingy *CLookupThingy::Lookup( name ) 
{
#if !defined(DEBUG)
   if (!this)
   {
      return NULL;
   }
#endif
   // else do the lookup code...
}

Таким образом, он ничего не сломает в производственном коде, давая вам возможность протестировать его в режиме отладки.

@Crashworks: Просто убедитесь, что вы определяете NDEBUG в своем режиме выпуска. Это не всегда дано:stackoverflow.com/questions/5354314/... Ben Hocking
Я только что добавил утверждение, которое немедленно отключается в режиме отладки, так что, по крайней мере, мы можем постепенно обнаруживать и исправлять все случаи по мере их появления во внутреннем тестировании. Crashworks
6

умный! = мудрый.

поиск всех сайтов вызовов без каких-либо инструментов рефакторинга должен быть достаточно простым; как-нибудь разбить GetLookup (), чтобы он не компилировался (например, изменить подпись), чтобы вы могли статически идентифицировать злоупотребление. затем добавьте функцию с именем DoLookup (), которая делает то, что все эти хаки делают прямо сейчас.

9

поскольку не виртуальные функции обычно либо встроены, либо преобразуются в функции, не являющиеся членами, и принимают «this» в качестве параметра. Однако в стандарте конкретно говорится, что вызов нестатической функции-члена вне времени жизни объекта не определен, а время жизни объекта определяется как начало, когда память для объекта была выделена, а конструктор завершился, если он имеет нетривиальная инициализация.

Стандарт делает исключение из этого правила только для вызовов, сделанных самим объектом во время строительства или разрушения, но даже в этом случае следует соблюдать осторожность, поскольку поведение виртуальных вызовов может отличаться от поведения в течение времени жизни объекта.

TL: DR: Я бы убил его огнем, даже если на очистку всех сайтов вызовов уйдет много времени.

12

возвращающих NULL, вам лучше исправить код, который вызывает методы с использованием указателя NULL. Сначала замени

if (!this) return NULL;

с участием

if (!this) {
  // TODO(Crashworks): Replace this case with an assertion on July, 2012, once all callers are fixed.
  printf("Please mail the following stack trace to myemailaddress. Thanks!");
  print_stacktrace();
  return NULL;
}

Теперь продолжайте работу, но исправьте их, когда они начнут работать. Замените:

GetLookup(x)->Lookup(y)...

с участием

convert_to_proxy(GetLookup(x))->Lookup(y)...

Где convert_to_proxy возвращает указатель без изменений, если только он не равен NULL, и в этом случае он возвращает FailedLookupObject, как в моем другом ответе.

А как же бедняга, который получает это сообщение об ошибке, когда пытается сделать что-то полезное, например, обработать важный заказ или продать свои акции до того, как цена упадет! Послание к выпускникам компьютерных программ - программы написаны так, чтобы ими не восхищались. James Anderson
Что должен делать какой-то бедный конечный пользователь, когда сталкивается с этим сообщением! Он, вероятно, придет к выводу, что программа не работает, и вернется к некоторой ручной процедуре, пока не будет найдено какое-либо программное обеспечение замены от другого поставщика. Вы вводите реальные ошибки, чтобы поймать воображаемые ошибки! James Anderson
Замените его какой-нибудь функцией, которая автоматически отправляет в стек трассировку. Не бросайте это в лицо пользователя. CodingBarfield
@JamesAnderson: программа работает как прежде. В идеале, кто-то может исправить все звонки прямо сейчас. Если это не может произойти, то следующая лучшая вещь, чтобы исправить проблемы, как вы их найдете. Это не имеет ничего общего с тщеславием программы. Neil G
Затем @Crashworks настраивает клиент / сервер так, чтобы время от времени он загружал журнал ошибок только в том случае, если сервер не занят и только в этой ситуации. Вы можете даже распространить это на процент ключевых пользователей, чтобы загружаемая сумма была меньше, и вы могли использовать ту же группу для бета-тестирования и т. Д. CodingBarfield
8

вероятно, будут более агрессивно оптимизированы в случаях формально неопределенного поведения. Я бы не беспокоился о существующих развертываниях (где вы знаете поведение, которое фактически реализовал компилятор), но это должно быть исправлено в исходном коде на случай, если вы когда-либо используете другой компилятор или другую версию.

Как насчет оценки поведения нового компилятора, а затем принятия решения на основе результатов? Robert Harvey♦
@RobertHarvey: Если бы существовал хороший набор тестов, ошибки такого рода не могли бы появиться. Ben Voigt
Запустив его в сравнении с уже существующим набором модульных тестов, конечно, используя желаемые оптимизации. Или путем тестирования дыма в полевых условиях. Robert Harvey♦
@ Роберт: Как вы предлагаете сделать это, учитывая, что когда он действительно ломается, он, вероятно, делает это в очень специфических обстоятельствах (возможно, встроенные функции-члены, когда оптимизация включена). Ben Voigt
4

это будет беспокоить вас через год. Как вы указали, его изменение почти наверняка приведет к ошибкам, но вы можете начать с сохраненияreturn NULL функциональности, добавьте немного логов, дайте ему поработать в дикой природе в течение нескольких недель и найдите, сколько раз его ударили?

"изменение его почти наверняка внесет некоторые ошибки«Нет, ошибки есть в любом случае - изменение их только сделает ошибки заметными, вместо того, чтобы смести их под ковер и притвориться, что их не существует. ildjarn
1

вы должны потерпеть неудачу как можно раньше, чтобы предупредить вас о проблемах. В этом случае я бы бесцеремонно удалил все случаиif(!this) Я мог бы найти.

Лучше:assert(this). Ben Voigt
@ Роберт: Для некоторого определения "работы" ... Если!this когда-либо оценивается как истинное, это просто означает, что в другом месте есть ошибка, над которой был бандой; Я бы выступил за исправлениереальный ошибка, и я думаю, что это то, что защищает этот ответ. ildjarn
@ildjarn: определение «работает» в вопросе. Вынимаяif(!this!) везде почти наверняка создаст библиотеку, которая делаетне работай. Robert Harvey♦
Так как же тогда вы оправдываете время, потраченное на исправление ранее работающей библиотеки, которая теперь взрывается для каждого пользователя? Это действительно стоит риска, времени и усилий? Robert Harvey♦
Я не хотел бы жить с неопределенностью. Это тихая ошибка, котораяявляется происходит. Вы должны найти это как можно скорее. Mooing Duck

Похожие вопросы