Support to emit extern variables debuginfo with "-fstandalone-debug"

[Jac1494]

Summary

Consider below testcases,

$cat shlib.c

int var;
int test()
{ return var++; }

$cat test.c

extern int test();
extern int var;
int main()
{ var++; printf("%d\n",test()); }

$clang -fpic shlib.c -g -shared -o libshared.so
$clang -L`pwd` test.c -lshared -g
$export LD_LIBRARY_PATH=pwd

$ gdb a.out
Reading symbols from a.out...
(gdb) b main
Breakpoint 1 at 0x400718: file test.c, line 6.
(gdb) pt var
type = <data variable, no debug info>

As per above debugging ,for global extern variable var type info is missing because variable DebugInfo is not there in executable.
This is the case when sharelib is not yet loaded.
LLVM is not adding debuginfo for externs with -g because this debug_info may increase final binary size.
And Even it fails to do so with -fstandalone-debug option.

As part of this fix
We emit debug info for declaration of global extern variable
with option -fstandalone-debug along with -g.

[dblaikie]

Do you have any particular users/use case for this?

Might be best as two separate patches (one for the LLVM change, one for the Clang change) with tests.

[Jac1494]

Separate clang patch with test cases .

[Jac1494]

In D71451#1786517, @dblaikie wrote:

Do you have any particular users/use case for this?

A case when shared library built without debug info
and executable with debug info. And while debugging we want to know the types of extern.

[dblaikie]

In D71451#1787729, @Jac1494 wrote:

In D71451#1786517, @dblaikie wrote:

Do you have any particular users/use case for this?

A case when shared library built without debug info
and executable with debug info. And while debugging we want to know the types of extern.

OK - I was/am trying to clarify you have a particular user/need for this, since no one's had one for many years, so I'm just slightly surprised.

@aprantl - you folks might want to check what the growth of this is like since apple platforms defaults to standalone-debug.
@Jac1494 - have you made any measurements of the size increase of this change? Perhaps a self-host build of clang?

[Jac1494]

@Jac1494 - have you made any measurements of the size increase of this change? Perhaps a self-host build of clang?

With change(default) and without change size diffrence given below ,in that only debug section size is changed.

Without change:-(default)
$size -A -d build_debug_withoutfix/bin/clang-10
....
....
.comment 159 0
.debug_str 3999832 0
.debug_loc 941 0
.debug_abbrev 12754 0
.debug_info 2225482 0
.debug_ranges 46672 0
.debug_line 153741 0
.note.gnu.gold-version 28 0
Total 6835098

With change:-

$size -A -d build_debug_withfix/bin/clang-10
.....
.....
.comment 159 0
.debug_str 3999775 0
.debug_loc 941 0
.debug_abbrev 12746 0
.debug_info 2225458 0
.debug_ranges 46672 0
.debug_line 153717 0
.note.gnu.gold-version 28 0
Total 6834985

[dblaikie]

In D71451#1791314, @Jac1494 wrote:

@Jac1494 - have you made any measurements of the size increase of this change? Perhaps a self-host build of clang?

With change(default) and without change size diffrence given below ,in that only debug section size is changed.

Without change:-(default)
$size -A -d build_debug_withoutfix/bin/clang-10
....
....
.comment 159 0
.debug_str 3999832 0
.debug_loc 941 0
.debug_abbrev 12754 0
.debug_info 2225482 0
.debug_ranges 46672 0
.debug_line 153741 0
.note.gnu.gold-version 28 0
Total 6835098

With change:-

$size -A -d build_debug_withfix/bin/clang-10
.....
.....
.comment 159 0
.debug_str 3999775 0
.debug_loc 941 0
.debug_abbrev 12746 0
.debug_info 2225458 0
.debug_ranges 46672 0
.debug_line 153717 0
.note.gnu.gold-version 28 0
Total 6834985

Am I reading this right that the data would suggest that enabling this feature /reduces/ the size of debug info sections? That doesn't sound right - can you explain why that would be the case? (perhaps the data is incorrect/it wasn't built with -fstandalone-debug?)

[Jac1494]

Am I reading this right that the data would suggest that enabling this feature /reduces/ the size of debug info sections? That doesn't sound right - can you explain why that would be the case? (perhaps the data is incorrect/it wasn't built with -fstandalone-debug?)

Hi @dblaikie sorry for incorrect data.

Updated result are as per below:

Without "-fstandalone-debug" option :-
$size -A -d build_debug_withoutstandalone/bin/clang-10
...
...
.comment 159 0
.debug_str 2655675 0
.debug_loc 941 0
.debug_abbrev 10761 0
.debug_info 1526674 0
.debug_ranges 46672 0
.debug_line 149807 0
.note.gnu.gold-version 28 0
Total 4786206

With "-fstandalone-debug" option :-
$size -A -d build_debug_withstandalone/bin/clang-10
...
...
.comment 159 0
.debug_str 3997839 0
.debug_loc 941 0
.debug_abbrev 12746 0
.debug_info 2225392 0
.debug_ranges 46672 0
.debug_line 153779 0
.note.gnu.gold-version 28 0
Total 6833045

[dblaikie]

In D71451#1793828, @Jac1494 wrote:

Am I reading this right that the data would suggest that enabling this feature /reduces/ the size of debug info sections? That doesn't sound right - can you explain why that would be the case? (perhaps the data is incorrect/it wasn't built with -fstandalone-debug?)

Hi @dblaikie sorry for incorrect data.

Updated result are as per below:

Without "-fstandalone-debug" option :-
$size -A -d build_debug_withoutstandalone/bin/clang-10
...
...
.comment 159 0
.debug_str 2655675 0
.debug_loc 941 0
.debug_abbrev 10761 0
.debug_info 1526674 0
.debug_ranges 46672 0
.debug_line 149807 0
.note.gnu.gold-version 28 0
Total 4786206

With "-fstandalone-debug" option :-
$size -A -d build_debug_withstandalone/bin/clang-10
...
...
.comment 159 0
.debug_str 3997839 0
.debug_loc 941 0
.debug_abbrev 12746 0
.debug_info 2225392 0
.debug_ranges 46672 0
.debug_line 153779 0
.note.gnu.gold-version 28 0
Total 6833045

Ah, that looks more like what I'd expect - so that's a 42% increase in the final linked binary size? Yeah, that seems somewhat unlikely to be what anyone would want to enable, though I could be wrong (@aprantl @JDevlieghere - you folks use standalone-debug by default, have any opinions on this proposed change to standalone-debug behavior?)

Could you audit some files & see what sort of things are going in there? It might be that a more nuanced definition of "used" is needed.

For instance, does this global variable get emitted?

extern int i;
inline int f1() {
  return i;
}
void f2() {
}

'i' is used according to the AST, but it's not used even statically by this translation unit - f1 is never called here. So if this patch currently emits a declaration of 'i' then it shows one way there's some room for improvement - specifically I'd motivate the declaration emission by the IR usage itself - does the LLVM IR end up with a declaration of 'i' in it (for the purposes of loading/storing to it, etc) then add a declaration. This won't catch all cases (eg: "extern const int i = 7; int f() { int x[i]; ... }" - there won't be any use of 'i' in the IR in that case), but would potentially be a workable tradeoff for now. Catching the "use of a global in a constant context" is part of a much broader challenge - to get that and some other existing cases like it right, we'd need some kind of reachability analysis of usage. That analysis could benefit other areas of debug info too, but is probably a lot more work.

[Jac1494]

ping

[aprantl]

In D71451#1816269, @Jac1494 wrote:

ping

I'd be curious to the answer to David's questions. If the size increase is because of unused extern variables coming in from libc or something then it doesn't seem worth the cost.

clang/lib/CodeGen/CGDebugInfo.cpp

4598

nit:

if (DebugKind < clang::codegenoptions::FullDebugInfo)
  return

[Jac1494]

I'd be curious to the answer to David's questions. If the size increase is because of unused extern variables coming in from libc or something then it doesn't seem worth the cost.

For above case clang size is increase because ,it is difference between clang build without "-fstandalone-debug" option and clang build with "-fstandalone-debug" option and both build contain change D71451 and D71599 . So for clang build with "-fstandalone-debug" option size will be more because it will add debuginfo.

And to check impact of my change on clang i have build clang with and without D71451 and D71599 change(testcases are not included).

Size of clang without D71451 and D71599 change and with option "-fstandalone-debug":-

…
.comment 159 0
.debug_str 3994952 0
.debug_loc 941 0
.debug_abbrev 12754 0
.debug_info 2223641 0
.debug_ranges 46592 0
.debug_line 153901 0
.note.gnu.gold-version 28 0
Total 6827932

Size of clang with D71451 and D71599 change and with option "-fstandalone-debug":-

…
.comment 159 0
.debug_str 3994894 0
.debug_loc 941 0
.debug_abbrev 12746 0
.debug_info 2223617 0
.debug_ranges 46592 0
.debug_line 153865 0
.note.gnu.gold-version 28 0
Total 6827806

Size of clang with D71451 and D71599 is reduced.

This results are with latest source and with self-host build of clang. First I have build clang with Release mode and using that clang I have build clang with debug mode with below options

“cmake -DLLVM_ENABLE_PROJECTS=clang -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER=$(which clang) -DCMAKE_CXX_COMPILER=$(which clang++) -DLLVM_TARGETS_TO_BUILD="X86" -DBUILD_SHARED_LIBS=On -DCMAKE_CXX_FLAGS="-fstandalone-debug" -DCMAKE_C_FLAGS="-fstandalone-debug" -DCMAKE_INSTALL_PREFIX=/home/bft/Jaydeep/latest_llvm/llvm-project/install_withhstandalone ../llvm”

[dblaikie]

In D71451#1822497, @Jac1494 wrote:

I'd be curious to the answer to David's questions. If the size increase is because of unused extern variables coming in from libc or something then it doesn't seem worth the cost.

For above case clang size is increase because ,it is difference between clang build without "-fstandalone-debug" option and clang build with "-fstandalone-debug" option and both build contain change D71451 and D71599 . So for clang build with "-fstandalone-debug" option size will be more because it will add debuginfo.

And to check impact of my change on clang i have build clang with and without D71451 and D71599 change(testcases are not included).

Size of clang without D71451 and D71599 change and with option "-fstandalone-debug":-

…
.comment 159 0
.debug_str 3994952 0
.debug_loc 941 0
.debug_abbrev 12754 0
.debug_info 2223641 0
.debug_ranges 46592 0
.debug_line 153901 0
.note.gnu.gold-version 28 0
Total 6827932

Size of clang with D71451 and D71599 change and with option "-fstandalone-debug":-

…
.comment 159 0
.debug_str 3994894 0
.debug_loc 941 0
.debug_abbrev 12746 0
.debug_info 2223617 0
.debug_ranges 46592 0
.debug_line 153865 0
.note.gnu.gold-version 28 0
Total 6827806

Size of clang with D71451 and D71599 is reduced.

Do you have any reason to believe these patches would reduce the size of the debug info? It seems like they only add more debug info, and don't remove anything - so we'd expect an increase in the size, certainly not a decrease. That means, to me, there's probably a mistake in the measurement somehow?

This results are with latest source and with self-host build of clang. First I have build clang with Release mode and using that clang I have build clang with debug mode with below options

“cmake -DLLVM_ENABLE_PROJECTS=clang -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER=$(which clang) -DCMAKE_CXX_COMPILER=$(which clang++) -DLLVM_TARGETS_TO_BUILD="X86" -DBUILD_SHARED_LIBS=On -DCMAKE_CXX_FLAGS="-fstandalone-debug" -DCMAKE_C_FLAGS="-fstandalone-debug" -DCMAKE_INSTALL_PREFIX=/home/bft/Jaydeep/latest_llvm/llvm-project/install_withhstandalone ../llvm”

[Jac1494]

Do you have any reason to believe these patches would reduce the size of the debug info? It seems like they only add more debug info, and don't remove anything - so we'd expect an increase in the size, certainly not a decrease. That means, to me, there's probably a mistake in the measurement somehow?

Yes, In clang binary DW_AT_linkage_name attribute is missing for "None" and "__default_lock_policy" two variable because of that size is reduced. For that fixed files are updated.
Now size is same with and without D71451 and D71599.

[dblaikie]

In D71451#1828647, @Jac1494 wrote:

Do you have any reason to believe these patches would reduce the size of the debug info? It seems like they only add more debug info, and don't remove anything - so we'd expect an increase in the size, certainly not a decrease. That means, to me, there's probably a mistake in the measurement somehow?

Yes, In clang binary DW_AT_linkage_name attribute is missing for "None" and "__default_lock_policy" two variable because of that size is reduced. For that fixed files are updated.
Now size is same with and without D71451 and D71599.

that still seems very surprising - this change is specifically intended to add more debug info, is it not? So do you have any ideas/theories as to why that's not showing up in the data?

[Jac1494]

that still seems very surprising - this change is specifically intended to add more debug info, is it not? So do you have any ideas/theories as to why that's not showing up in the data?

Yes, In clang and llvm extern variable are there inside header file ,So that it will not add debug info.And in test side only two test case(virtual-methods.ll, cl-options.c) are there which is using "-fstandalone-debug" option ,But in this test cases there is no extern variable.That's why size is same after this change.

[Jac1494]

clang/lib/CodeGen/CGDebugInfo.cpp

4598

@aprantl thanks for suggestion , change is updated.

[Jac1494]

In D71451#1817334, @aprantl wrote:

In D71451#1816269, @Jac1494 wrote:

ping

I'd be curious to the answer to David's questions. If the size increase is because of unused extern variables coming in from libc or something then it doesn't seem worth the cost.

@aprantl now size of clang is same with this change (D71451 and D71599).

[Jac1494]

ping

[aprantl]

If I'm reading the data correctly this means a < 1% increase in .debug_info size? If this is correct, I'm fine with the change.

[dblaikie]

If this feature treats a declaration in a header different from one in a primary source file - I think that's problematic from a debug info/debugging perspective (& perhaps a quirk of how this functionality was originally implemented for the kernel feature thingy (the name I forget right now)... one I didn't notice & should probably go check up on to see if it makes sense there/how to rationalize it). I think it'd be surprising to users if moving a declaration into/out of a source file/header changed what they could do in a debugger and/or significantly increased debug info size, etc.

clang/include/clang/Basic/TargetInfo.h

   /// Check the target is valid after it is fully initialized.
   virtual bool validateTarget(DiagnosticsEngine &Diags) const {
     return true;
   }
 
   virtual void setAuxTarget(const TargetInfo *Aux) {}
 
   /// Whether target allows debuginfo types for decl only variables.
-  virtual bool allowDebugInfoForExternalVar() const { return false; }
+  virtual bool allowDebugInfoForExternalVar() const { return true; }
 
 protected:
   /// Copy type and layout related info.
   void copyAuxTarget(const TargetInfo *Aux);
   virtual uint64_t getPointerWidthV(unsigned AddrSpace) const {
     return PointerWidth;
   }
   virtual uint64_t getPointerAlignV(unsigned AddrSpace) const {

clang/lib/CodeGen/CGDebugInfo.cpp

GV.reset(DBuilder.createGlobalVariableExpression(
       TemplateParameters, Align));
 }
 
 void CGDebugInfo::EmitExternalVariable(llvm::GlobalVariable *Var,
                                        const VarDecl *D) {
   assert(DebugKind >= codegenoptions::LimitedDebugInfo);
   if (D->hasAttr<NoDebugAttr>())
     return;
+  if (!(DebugKind == clang::codegenoptions::FullDebugInfo))
+    return;
 
   auto Align = getDeclAlignIfRequired(D, CGM.getContext());
   llvm::DIFile *Unit = getOrCreateFile(D->getLocation());
   StringRef Name = D->getName();
   llvm::DIType *Ty = getOrCreateType(D->getType(), Unit);
 
   llvm::DIScope *DContext = getDeclContextDescriptor(D);
   llvm::DIGlobalVariableExpression *GVE =

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

if (auto *SDMDecl = GV->getStaticDataMemberDeclaration()) {
 
     // Add line number info.
     addSourceLine(*VariableDIE, GV);
   }
 
   if (!GV->isDefinition())
     addFlag(*VariableDIE, dwarf::DW_AT_declaration);
   else
+  {
+    // Add location.
+    addLocationAttribute(VariableDIE, GV, GlobalExprs);
     addGlobalName(GV->getName(), *VariableDIE, DeclContext);
+  }
 
   if (uint32_t AlignInBytes = GV->getAlignInBytes())
     addUInt(*VariableDIE, dwarf::DW_AT_alignment, dwarf::DW_FORM_udata,
             AlignInBytes);
 
   if (MDTuple *TP = GV->getTemplateParams())
     addTemplateParams(*VariableDIE, DINodeArray(TP));
 
-  // Add location.
-  addLocationAttribute(VariableDIE, GV, GlobalExprs);
-
   return VariableDIE;
 }
 
 void DwarfCompileUnit::addLocationAttribute(
     DIE *VariableDIE, const DIGlobalVariable *GV, ArrayRef<GlobalExpr> GlobalExprs) {
   bool addToAccelTable = false;
   DIELoc *Loc = nullptr;
   Optional<unsigned> NVPTXAddressSpace;