[all-commits] [llvm/llvm-project] 997dc7: [debug-info][codegen] Prevent creation of self-ref...

[Felipe de Azevedo Piovezan via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 997dc7e00f49663b60a78e18df1dfecdf62a4172
      https://github.com/llvm/llvm-project/commit/997dc7e00f49663b60a78e18df1dfecdf62a4172
  Author: Felipe de Azevedo Piovezan <fpiovezan at apple.com>
  Date:   2023-02-20 (Mon, 20 Feb 2023)

  Changed paths:
    M clang/lib/CodeGen/CGDebugInfo.cpp
    M llvm/docs/LangRef.rst
    M llvm/lib/IR/Verifier.cpp
    A llvm/test/Verifier/disubprogram_declaration.ll

  Log Message:
  -----------
  [debug-info][codegen] Prevent creation of self-referential SP node

The function `CGDebugInfo::EmitFunctionDecl` is supposed to create a
declaration -- never a _definition_ -- of a subprogram. This is made
evident by the fact that the SPFlags never have the "Declaration" bit
set by that function.

However, when `EmitFunctionDecl` calls `DIBuilder::createFunction`, it
still tries to fill the "Declaration" argument by passing it the result
of `getFunctionDeclaration(D)`. This will query an internal cache of
previously created declarations and, for most code paths, we return
nullptr; all is good.

However, as reported in [0], there are pathological cases in which we
attempt to recreate a declaration, so the cache query succeeds,
resulting in a subprogram declaration whose declaration field points to
another declaration. Through a series of RAUWs, the declaration field
ends up pointing to the SP itself. Self-referential MDNodes can't be
`unique`, which causes the verifier to fail (declarations must be
`unique`).

We can argue that the caller should check the cache first, but this is
not a correctness issue (declarations are `unique` anyway). The bug is
that `CGDebugInfo::EmitFunctionDecl` should always pass `nullptr` to the
declaration argument of `DIBuilder::createFunction`, expressing the fact
that declarations don't point to other declarations. AFAICT this is not
something for which any reasonable meaning exists.

This seems a lot like a copy-paste mistake that has survived for ~10
years, since other places in this file have the exact same call almost
token-by-token.

I've tested this by compiling LLVMSupport with and without the patch, O2
and O0, and comparing the dwarfdump of the lib. The dumps are identical
modulo the attributes decl_file/producer/comp_dir.

[0]: https://github.com/llvm/llvm-project/issues/59241

Differential Revision: https://reviews.llvm.org/D143921