[PATCH] D91410: [llvm][clang][mlir] Add checks for the return values from Target::createXXX to prevent protential null deref

[Ella Ma via Phabricator via llvm-commits]

OikawaKirie added a comment.

In D91410#2400018 <https://reviews.llvm.org/D91410#2400018>, @tejohnson wrote:

> 



> ... the new checking is a mix of assert and fatal errors. Is that intended?

No. The added checks are based on the checks on other calls to the `Target::createXXX` functions in this file or other related files. If there are any fatal errors nearby (e.g. llvm/lib/LTO/ThinLTOCodeGenerator.cpp:581 vs 569), the check will be a  fatal error; and if there are any assertions (e.g. llvm/lib/CodeGen/LLVMTargetMachine.cpp:43,45,52 vs 60) or the calls are never checked (e.g. llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:300), the added check will be an assertion.

> If these are not likely due to user input issues, then perhaps they should all be assert so that they are compiled out in release compilers?

Since all these problems are reported by my static analyzer, I do not really know whether these checked pointers will actually be null when the code is executed. And I did not try to dynamically execute the program to check the problems either. But chances are that if the creator callbacks are not properly set or the called creator functions returns nullptr, the problem will happen. In my opinion, these problems may only happen during development. Therefore, I believe asserts can be sufficient to diagnose the problems.

If you think it would be better to use assertions instead of fatal errors, I will make an update on all `llvm/lib/xxx` files (or maybe all files). But before that, I'd prefer waiting for the replies from other reviewers on the remaining parts of the patch and making an update for all the suggestions.

Thank you.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91410/new/

https://reviews.llvm.org/D91410