Подтвердить что ты не робот

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

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

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

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

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

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

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

Изменить: Вид объективного ответа, который я ищу, похож на

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

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

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

4b9b3361

ОТВЕТЫ

Ответ 1

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

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

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

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

Ответ 2

Как и все undefined поведение

if (!this)
{
   return NULL;
}

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

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

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

Ответ 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;
}

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

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

с

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

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

Ответ 4

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

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

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

Ответ 5

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

Ответ 6

это то, что называется "умным и уродливым взломом". Примечание: умный!= мудрый.

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

Ответ 7

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

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

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

Ответ 8

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

Ответ 9

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

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

Ответ 10

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

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

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

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

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

Если вы придерживаетесь формулировки описания, используйте this (который включает проверку, является ли он нулевым указателем) поведение undefined. По сути, это "бомба замедленного действия". Тем не менее, я бы не счел разумным считать эту проблему как с практической точки зрения, так и с логической точки зрения.

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

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

Ответ 11

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