[clang][DebugInfo] Emit DISubprogram for extern functions with reserved names

[eddyz87]

Summary

Callsite DISubprogram entries are not generated for:

• builtin functions;
• external functions with reserved names (e.g. names starting from "__").

This limitation was added by the commit [1] as a workaround for the
situation described in [2] that triggered the IR verifier error.
The goal of the present commit is to lift this limitation by adjusting
the IR verifier logic.

The logic behind [1] is to avoid the following situation:

• a DISubprogram is added for some builtin function;
• there is some location where this builtin is also emitted by a transformation (w/o debug location);
• the Verifier::visitCallBase sees a call to a function with DISubprogram but w/o debug location and emits an error.

Here is an updated example of such situation taken from [2]:

extern "C" int memcmp(void *, void *, long);

struct a { int b; int c; int d; };

struct e { int f[1000]; };

bool foo(e g, e &h) {
  // DISubprogram for memcmp is created here when [1] is commented out
  return memcmp(&g, &h, sizeof(e));
}

bool bar(a &g, a &h) {
  // memcmp might be generated here by MergeICmps
  return g.b == h.b && g.c == h.c && g.d == h.d;
}

This triggers the verifier error when:

• compiled for AArch64: clang++ -c -g -Oz -target aarch64-unknown-linux-android21 test.cpp;
• [1] check is commented out.

Instead of forbidding generation of DISubprogram entries as in [1]
one can instead adjust the verifier to additionally check if callee
has a body. Functions w/o bodies cannot be inlined and thus verifier
warning is not necessary.

E.g. llvm::InlineFunction requires functions for which
GlobalValue::isDeclaration() == false.

[1] 568db780bb7267651a902da8e85bc59fc89aea70
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=1022296

[eddyz87]

Rebase

[eddyz87]

Hello Adrian, I've noticed that you was tagged as a reviewer in a somewhat related revision: https://reviews.llvm.org/D133060, so decided to tag you here.
Could you please take a look?

[yonghong-song]

@aprantl The following example shows the difference between gcc and clang w.r.t. reserved names.

[$ ~/tmp3] cat t.c
extern void *__bpf_kptr_new(int, int, void *)
__attribute__((section(".ksyms")));
#define bpf_kptr_new(x) __bpf_kptr_new(x, 0, 0)

struct foo {
        int data;
};

int main(void)
{
        struct foo *f;

        f = bpf_kptr_new(0);
        return f->data;
}
[$ ~/tmp3] clang -target bpf -O2 -g -c t.c
[$ ~/tmp3] llvm-dwarfdump t.o | grep bpf_kptr_new
[$ ~/tmp3] gcc -O2 -g -c t.c
[$ ~/tmp3] llvm-dwarfdump t.o | grep bpf_kptr_new
                  DW_AT_abstract_origin (0x000000a3 "__bpf_kptr_new")
                DW_AT_linkage_name      ("__bpf_kptr_new")
                DW_AT_name      ("__bpf_kptr_new")
[$ ~/tmp3]

If not using reserved name, the function will appear in debuginfo/dwarf:

[$ ~/tmp3] cat t1.c
extern void *bpf_kptr_new_(int, int, void *)
__attribute__((section(".ksyms")));
#define bpf_kptr_new(x) bpf_kptr_new_(x, 0, 0)

struct foo {
        int data;
};

int main(void)
{
        struct foo *f;

        f = bpf_kptr_new(0);
        return f->data;
}
[$ ~/tmp3] clang -target bpf -O2 -g -c t1.c
[$ ~/tmp3] llvm-dwarfdump t1.o | grep bpf_kptr_new
                DW_AT_name      ("bpf_kptr_new_")
[$~/tmp3] gcc -O2 -g -c t1.c
[$ ~/tmp3] llvm-dwarfdump t1.o | grep bpf_kptr_new
                  DW_AT_abstract_origin (0x000000a3 "bpf_kptr_new_")
                DW_AT_linkage_name      ("bpf_kptr_new_")
                DW_AT_name      ("bpf_kptr_new_")
[$ ~/tmp3]

In BPF, we need the above extern function in debuginfo. This happens when a bpf program is calling a function defined in kernel. For example,
in kernel we have function

int tcp_drop(struct sk_buff *skb, ...) { ...
}
/* allow tcp_drop() function accessible to bpf programs */

In bpf program, we could have

extern int tcp_drop(struct sk_buff *sbk, ...);
BPF_PROG(...) {
    tcp_drop(...)

BPF verifier will need to check whether the signature of tcp_drop() in bpf program matched the one in kernel or not. In kernel, we DO have some global functions with '__' prefix in the function name. Although no reserved function name works, we want all legal names working so user won't be surprised why some kernel functions are not available to them.

}

[aprantl]

I think this is reasonable. Ideally the LLVM and Clang patches should be two independent commits.
Please wait a few days for others to chime in though. Do you have any idea how much this will grow the debug info for something like, e.g., clang?

[eddyz87]

In D136041#3862741, @aprantl wrote:

I think this is reasonable. Ideally the LLVM and Clang patches should be two independent commits.
Please wait a few days for others to chime in though. Do you have any idea how much this will grow the debug info for something like, e.g., clang?

Thanks for the review,
I will split the patch and provide the measurements, most likely tomorrow.

[dblaikie]

Hmm - this does mean linking IR can produce invalid code, though, right (you link in a definition of the function, so what was valid is now invalid - because it now has a definition, can be inlined, etc)? Is that new? concerning?

[yonghong-song]

In D136041#3863748, @dblaikie wrote:

Hmm - this does mean linking IR can produce invalid code, though, right (you link in a definition of the function, so what was valid is now invalid - because it now has a definition, can be inlined, etc)? Is that new? concerning?

@dblaikie do you have a concrete example to illustrate the problem?

[eddyz87]

In D136041#3863748, @dblaikie wrote:

Hmm - this does mean linking IR can produce invalid code, though, right (you link in a definition of the function, so what was valid is now invalid - because it now has a definition, can be inlined, etc)? Is that new? concerning?

As far as I understand this discussion the check in question is "best effort".
My change further narrows conditions when verifier would report an error, thus it should not add any new failures to the existing code.
But yes, hypothetically there might be a situation when old version of the check would have caught a miss-behaving transformation at an earlier stage (before linking IR rather then after).

[eddyz87]

Split the original commit in two to separate clang and llvm parts.

[dblaikie]

In D136041#3864171, @yonghong-song wrote:

In D136041#3863748, @dblaikie wrote:

Hmm - this does mean linking IR can produce invalid code, though, right (you link in a definition of the function, so what was valid is now invalid - because it now has a definition, can be inlined, etc)? Is that new? concerning?

@dblaikie do you have a concrete example to illustrate the problem?

One translation unit with a call to a function declaration, that call has no !dbg attachment.
Another translation unit with the definition of that declared function.

As-is, this would not cause any verifier error or backend debug info assertion.
Link the two together and now it's a verifier error - and, depending on the contents of the function definition, maybe a backend debug info assertion.

In D136041#3864782, @eddyz87 wrote:

In D136041#3863748, @dblaikie wrote:

Hmm - this does mean linking IR can produce invalid code, though, right (you link in a definition of the function, so what was valid is now invalid - because it now has a definition, can be inlined, etc)? Is that new? concerning?

As far as I understand this discussion the check in question is "best effort".

Fair point

My change further narrows conditions when verifier would report an error, thus it should not add any new failures to the existing code.

No, the opposite.

But yes, hypothetically there might be a situation when old version of the check would have caught a miss-behaving transformation at an earlier stage (before linking IR rather then after).

Right - now things that were invalid categorically, are now valid, unless you link them. Which is a bit tricky.

Ah well, as you say - it's best effort.

[eddyz87]

In D136041#3862741, @aprantl wrote:

... Do you have any idea how much this will grow the debug info for something like, e.g., clang?

I made the measurements but I'm not sure what to make of these numbers (e.g. is it good or bad).
There were no changes in executable size for Clang (in Debug mode, I'll try in Release mode a bit later), so I tried the Linux kernel with debug symbols enabled instead.
Here are executable sizes in three modes:

• (main) w/o my changes;
• (a) with these two commits;
• (b) with these two commits but with CalleeDecl->getBuiltinID() != 0 in CGDebugInfo::EmitFuncDeclForCallSite put back.

	a	b	main	a - main	b - main
vmlinux size (bytes)	667,827,456	667,365,792	663,996,200	+3,831,256	+3,369,592
Debug section size (bytes)	565,548,585	565,086,931	561,717,339	+3,831,246	+3,369,592
#call site DIEs	311,217	303,909	242,933	+68,284	+60,976
#call site parameter DIEs	369,405	361,080	301,738	+67,667	+59,342

The BTF use-case does not care about the builtin functions, so I can change this revision to variant (b) to save some space.
What do you think?

[probinson]

llvm/lib/IR/Verifier.cpp

3472

(Interposable functions are not inlinable, neither are functions without
definitions.)

[eddyz87]

Comment fixed.

[MaskRay]

Nice. This also fixes DW_AT_call_all_calls for functions which call builtins/reserved name functions.

llvm/test/Verifier/callsite-dbgloc.ll

52

to not match other functions named @i*

[MaskRay]

llvm/lib/IR/Verifier.cpp

3475

Removing this check does not break any check-llvm check-clang test.

[eddyz87]

Fixed test case to fail if !Call.getCalledFunction()->isDeclaration() condition is removed in Verifier::visitCallBase.

[eddyz87]

llvm/lib/IR/Verifier.cpp

3475

Well, that's embarrassing, thank you for catching it!
I've updated llvm/test/Verifier/callsite-dbgloc.ll by adding !dbg metadata for @i. Know the test fails if the check above is commented out.

clang/lib/CodeGen/CGDebugInfo.cpp

void CGDebugInfo::EmitFuncDeclForCallSite(llvm::CallBase *CallOrInvoke,
   if (!CallOrInvoke)
     return;
   auto *Func = CallOrInvoke->getCalledFunction();
   if (!Func)
     return;
   if (Func->getSubprogram())
     return;
 
-  // Do not emit a declaration subprogram for a builtin, a function with nodebug
-  // attribute, or if call site info isn't required. Also, elide declarations
-  // for functions with reserved names, as call site-related features aren't
-  // interesting in this case (& also, the compiler may emit calls to these
-  // functions without debug locations, which makes the verifier complain).
-  if (CalleeDecl->getBuiltinID() != 0 || CalleeDecl->hasAttr<NoDebugAttr>() ||
+  // Do not emit a declaration subprogram for a function with nodebug
+  // attribute, or if call site info isn't required.
+  if (CalleeDecl->hasAttr<NoDebugAttr>() ||
       getCallSiteRelatedAttrs() == llvm::DINode::FlagZero)
     return;
-  if (CalleeDecl->isReserved(CGM.getLangOpts()) !=
-      ReservedIdentifierStatus::NotReserved)
-    return;
 
   // If there is no DISubprogram attached to the function being called,
   // create the one describing the function in order to have complete
   // call site debug info.
   if (!CalleeDecl->isStatic() && !CalleeDecl->isInlined())
     EmitFunctionDecl(CalleeDecl, CalleeDecl->getLocation(), CalleeType, Func);
 }
 

clang/test/CodeGen/debug-info-extern-call.c

 // DECLS-FOR-EXTERN-NOT: !DICompileUnit({{.*}}retainedTypes: !{{[0-9]+}}
 // DECLS-FOR-EXTERN: [[INT_TYPE:![0-9]+]] = !DIBasicType(name: "int",
 // DECLS-FOR-EXTERN: !DISubprogram(name: "fn1"
 // DECLS-FOR-EXTERN-SAME: type: [[FN1_TYPE:![0-9]+]],
 // DECLS-FOR-EXTERN: [[FN1_TYPE]] = !DISubroutineType(types: [[FN1_TYPES:![0-9]+]])
 // DECLS-FOR-EXTERN: [[FN1_TYPES]] = !{[[X_TYPE:![0-9]+]],
 // DECLS-FOR-EXTERN: [[X_TYPE]] = !DIDerivedType(tag: DW_TAG_typedef, name: "x",
 // DECLS-FOR-EXTERN-SAME: baseType: [[INT_TYPE]])
-// DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "memcmp"
-// DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "__some_reserved_name"
+// DECLS-FOR-EXTERN: !DISubprogram(name: "memcmp"
+// DECLS-FOR-EXTERN: !DISubprogram(name: "__some_reserved_name"
 
 // NO-DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "fn1"
 // NO-DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "memcmp"
 // NO-DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "__some_reserved_name"
 
 typedef int x;
 extern x fn1(int a, int b);
 extern int memcmp(const void *s1, const void *s2, unsigned long n);

llvm/lib/IR/Verifier.cpp

void Verifier::visitCallBase(CallBase &Call) {
 
   // Verify that callee and callsite agree on whether to use pointer auth.
   Check(!(Call.getCalledFunction() && FoundPtrauthBundle),
         "Direct call cannot have a ptrauth bundle", Call);
 
   // Verify that each inlinable callsite of a debug-info-bearing function in a
   // debug-info-bearing function has a debug location attached to it. Failure to
   // do so causes assertion failures when the inliner sets up inline scope info
-  // (Interposable functions are not inlinable).
+  // (Interposable functions are not inlinable, neither are functions without
+  //  definitions.)
   if (Call.getFunction()->getSubprogram() && Call.getCalledFunction() &&
       !Call.getCalledFunction()->isInterposable() &&
+      !Call.getCalledFunction()->isDeclaration() &&
       Call.getCalledFunction()->getSubprogram())
     CheckDI(Call.getDebugLoc(),
             "inlinable function call in a function with "
             "debug info must have a !dbg location",
             Call);
 
   if (Call.isInlineAsm())
     verifyInlineAsmCall(Call);

llvm/test/Verifier/callsite-dbgloc.ll

 
 ; Function Attrs: nounwind ssp uwtable
 define linkonce_odr void @k() #0 !dbg !20 {
 entry:
   call void (...) @i(), !dbg !21
   ret void, !dbg !22
 }
 
-declare void @i(...) #1
+declare !dbg !23 void @i(...) #1
 
 ; Function Attrs: nounwind ssp uwtable
 define void @g() #0 !dbg !11 {
 entry:
 ; Manually removed !dbg.
 ; CHECK: inlinable function call in a function with debug info must have a !dbg location
+; CHECK: @h()
   call void @h()
 ; CHECK-NOT: inlinable function call in a function with debug info must have a !dbg location
+; CHECK-NOT: @j()
   call void @j()
 ; CHECK: inlinable function call in a function with debug info must have a !dbg location
+; CHECK: @k()
   call void @k()
+; CHECK-NOT: inlinable function call in a function with debug info must have a !dbg location
+; CHECK-NOT: @i()
+  call void (...) @i()
   ret void, !dbg !13
 }
 
 ; Function Attrs: nounwind ssp uwtable
 define void @f() #0 !dbg !14 {
 entry:
   call void @g(), !dbg !15
   ret void, !dbg !16

 !15 = !DILocation(line: 4, column: 12, scope: !14)
 !16 = !DILocation(line: 4, column: 17, scope: !14)
 !17 = distinct !DISubprogram(name: "j", scope: !1, file: !1, line: 5, type: !8, isLocal: false, isDefinition: true, scopeLine: 5, isOptimized: false, unit: !0, retainedNodes: !2)
 !18 = !DILocation(line: 4, column: 12, scope: !17)
 !19 = !DILocation(line: 4, column: 17, scope: !17)
 !20 = distinct !DISubprogram(name: "k", scope: !1, file: !1, line: 6, type: !8, isLocal: false, isDefinition: true, scopeLine: 6, isOptimized: false, unit: !0, retainedNodes: !2)
 !21 = !DILocation(line: 4, column: 12, scope: !20)
 !22 = !DILocation(line: 4, column: 17, scope: !20)
+!23 = !DISubprogram(name: "i", scope: !1, file: !1, line: 1, type: !24, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
+!24 = !DISubroutineType(types: !25)
+!25 = !{null}