Насколько плохо "if (! This)" в функции-члене C ++?

Question

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

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

Представьте, что класс CLookupThingy имеет не виртуальную CThingy *CLookupThingy::Lookup( name ) функцию-член.Видимо, один из программистов в те ковбойские времена сталкивался со многими сбоями, когда NULL CLookupThingy * 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, где иногда он компилирует не виртуальные члены как прямые ветви с неявным параметром, а иногдав качестве указателей на функции смещения класса. "

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

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

Answer 1

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

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

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

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

Answer 2

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

if (!this)
{
   return NULL;
}

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

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

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

Answer 3

Если у вас много функций GetLookup, возвращающих 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;
}

Теперь продолжайте выполнять другие работы, но исправьте их по мере их появления.с

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

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

Answer 4

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

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

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

Answer 5

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

Answer 6

это то, что называется «умный и безобразный хакер».примечание: умный! = мудрый.

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

Answer 7

В этом случае я бы предложил удалить проверку NULL из функции-члена и создать функцию, не являющуюся членом

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

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

Answer 8

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

Answer 9

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

Такое поведение, скрывающее ошибки, на самом деле тоже не то, на что вы хотите положиться.Представьте, что вы полагаетесь на это поведение (т. Е. , не имеет значения, допустимы ли мои объекты, оно все равно будет работать! ), а затем, при некоторой опасности, компилятор оптимизирует оператор 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 взорвется, другое тоже.Таким образом, проверка устаревает, потому что программа уже аварийно завершает работу.

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

Answer 10

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

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