[PATCH] D29203: Change debug-info-for-profiling from a TargetOption to a function attribute.

[Dehao Chen via Phabricator via llvm-commits]

danielcdh added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1138
+    HasDebugInfoForProfiling=true;
+
   // If there's no debug info for the function we're not going to do anything.
----------------
mehdi_amini wrote:
> probinson wrote:
> > danielcdh wrote:
> > > mehdi_amini wrote:
> > > > danielcdh wrote:
> > > > > mehdi_amini wrote:
> > > > > > ahatanak wrote:
> > > > > > > Is debug-info-for-profiling used to turn on and off debug info for profiling on a per-function basis? I'm not very familiar with this function, but it looks like HasDebugInfoForProfiling is initialized to false in the constructor and then it will always be true after beginFunction is called on a function that has attribute debug-info-for-profiling=true.
> > > > > > Yep that seems broken to me. If foo has the attribute and not bar, we'll emit the profiling debug info for bar if foo is processed first.
> > > > > The issue is that I cannot check for function's attributes in DwarfUnit::applySubprogramAttributes, any suggestions on how I can pass down this attribute to that function?
> > > > Why not do here `HasDebugInfoForProfiling = MF->getFunction()->debugInfoForProfiling();` ?
> > > Because the llvm::DwarfDebug::beginFunction is called first before llvm::DwarfUnit::applySubprogramAttributes are called.
> > > 
> > > E.g. you have f1, f2, f3
> > > 
> > > the call sequence is:
> > > beginFunction(f1)
> > > beginFunction(f2)
> > > beginFunction(f3)
> > > applySubprogramAttributes(subprogramof_f1)
> > > applySubprogramAttributes(subprogramof_f2)
> > > applySubprogramAttributes(subprogramof_f3)
> > > 
> > > The current logic is, if there is any function that has HasDebugInfoForProfiling=true, then all function will have it.
> > All the more reason to put it at module or CU level, rather than function level.
> If the behavior you want is to have it enabled for the module, then yes like @probinson  says a module level construct would make more sense.
> 
> I don't enough about `-gmlt` to know what we would expect if we have a.cpp and b.cpp and we build with LTO one with -g and the other one with -gmlt ?
> 
Updated to use module flag for this.


================
Comment at: test/DebugInfo/Inputs/gmlt_profiling.ll:14
+; CHECK: DW_AT_decl_file
+; CHECK: DW_AT_decl_line
+
----------------
probinson wrote:
> Writing checks for dwarfdump output is tricky because you're really trying to match several attribute lines within the same DWARF entry.  So here you want something like this:
> ```
> CHECK: DW_TAG_subprogram
> CHECK-NOT: {{DW_TAG|NULL}}
> CHECK: DW_AT_name {{.*}} "f1"
> CHECK-NEXT: DW_AT_decl_file
> CHECK-NEXT: DW_AT_decl_line
> ```
> 
> This assumes that name/file/line appear in that exact sequence, but there are lots of debug-info tests with that kind of assumption.  Unless/until we get a fancier debug-info matcher this is the kind of thing we have to do.
Done, but the CHECK-NOT cannot be used because we have DW_AT_low_pc and DW_AT_high_pc in between.


https://reviews.llvm.org/D29203