CGDebugInfo: Drop Loc.isInvalid() special case from getLineNumber

[MaskRay]

Summary

getLineNumber() picks CurLoc if the parameter is invalid. This appears to
mainly work around missing SourceLocation information for some constructs, but
sometimes adds unintended locations.

• For CodeGenObjC/debug-info-blocks.m, CurLoc has been advanced to the closing brace. The debug line of ImplicitVarParameter is set to the line of } because this implicit parameter has an invalid SourceLocation. The debug line is a bit arbitrary - perhaps the location of ^{ is better.
• (getOrCreateRecordFwdDecl) In clang/tools/driver/driver.cpp, the line of a #include is attached to DW_AT_decl_file/DW_AT_decl_line of a forward declaration __va_list_tag deep in the included hierarchy.

Drop the special case to make getLineNumber less magic and add CurLoc fallback in its callers instead.

Tested with stage 2 -DCMAKE_BUILD_TYPE=Debug clang, byte identical.

[dblaikie]

Any particular bug you're trying to fix? (this looks like it changes the debug locations, so it would need tests)

Not necessarily clear that these are better debug locations?

[MaskRay]

In D94391#2491178, @dblaikie wrote:

Any particular bug you're trying to fix? (this looks like it changes the debug locations, so it would need tests)

Not necessarily clear that these are better debug locations?

No particular bug. CGDebugInfo::getLineNumber returning CurLoc just looks strange. (The CurLoc may be far below (irrelevant) when getLineNumber is called.)

This patch is hopefully NFC. Dropping CurLoc from getLineNumber(RD->getLocation().isValid() ? RD->getLocation() : CurLoc) will cause a difference, which may reveal missing SourceLocation information in ObjC constructs.

[dblaikie]

In D94391#2491229, @MaskRay wrote:

In D94391#2491178, @dblaikie wrote:

Any particular bug you're trying to fix? (this looks like it changes the debug locations, so it would need tests)

Not necessarily clear that these are better debug locations?

No particular bug. CGDebugInfo::getLineNumber returning CurLoc just looks strange. (The CurLoc may be far below (irrelevant) when getLineNumber is called.)

This patch is hopefully NFC. Dropping CurLoc from getLineNumber(RD->getLocation().isValid() ? RD->getLocation() : CurLoc) will cause a difference, which may reveal missing SourceLocation information in ObjC constructs.

Ah, OK. Yeah, I don't have strong feelings about this - a bit hesitant to make the change without a stronger motivation though. Perhaps other reviewers have some thoughts to contribute (also maybe @JDevlieghere ).

[MaskRay]

In D94391#2493978, @dblaikie wrote:

In D94391#2491229, @MaskRay wrote:

In D94391#2491178, @dblaikie wrote:

Any particular bug you're trying to fix? (this looks like it changes the debug locations, so it would need tests)

Not necessarily clear that these are better debug locations?

No particular bug. CGDebugInfo::getLineNumber returning CurLoc just looks strange. (The CurLoc may be far below (irrelevant) when getLineNumber is called.)

This patch is hopefully NFC. Dropping CurLoc from getLineNumber(RD->getLocation().isValid() ? RD->getLocation() : CurLoc) will cause a difference, which may reveal missing SourceLocation information in ObjC constructs.

Ah, OK. Yeah, I don't have strong feelings about this - a bit hesitant to make the change without a stronger motivation though. Perhaps other reviewers have some thoughts to contribute (also maybe @JDevlieghere ).

Using CurLoc in getLineNumber is not ideal. Trying to fix one call site reveals a probably unsatisfactory debug location for ObjC ImplicitParamDecl.
The location is set to the closing brace. I do not investigate further as I know nearly nothing about ObjC...

--- a/clang/test/CodeGenObjC/debug-info-blocks.m
+++ b/clang/test/CodeGenObjC/debug-info-blocks.m
@@ -65,7 +65,7 @@ static void run(void (^block)(void))
       // CHECK-DAG: !DILocalVariable(arg: 2, scope: ![[COPY_SP]], {{.*}}, flags: DIFlagArtificial)
       // CHECK-DAG: !DILocalVariable(arg: 1, scope: ![[DESTROY_SP]], {{.*}}, flags: DIFlagArtificial)
       run(^{
-          // CHECK-DAG: ![[SELF]] = !DILocalVariable(name: "self", scope:{{.*}}, line: [[@LINE+4]],
+          // CHECK-DAG: ![[SELF]] = !DILocalVariable(name: "self", scope:{{.*}}, line: [[@LINE-7]],
           // CHECK-DAG: ![[D]] = !DILocalVariable(name: "d", scope:{{.*}}, line: [[@LINE+1]],
           NSMutableDictionary *d = [[NSMutableDictionary alloc] init]; 
           ivar = 42 + (int)[d count];

I think if the call sites are fixed, we can avoid future unintended introduction of undesired source locations.

[MaskRay]

I removed CurLoc from call sites and tried a stage 2 build. There is such a difference:

0x00062228:   DW_TAG_structure_type
                DW_AT_calling_convention        (DW_CC_pass_by_value)
                DW_AT_name      ("__va_list_tag")
                DW_AT_byte_size (0x18)
                DW_AT_decl_file ("/home/maskray/llvm/clang/tools/driver/driver.cpp")
                DW_AT_decl_line (23)

driver.cpp:23 is a #include. So this looks strange. The DW_AT_decl_file/DW_AT_decl_line attributes are undesired due to CurLoc getLineNumber.

[dblaikie]

In D94391#2494316, @MaskRay wrote:

I removed CurLoc from call sites and tried a stage 2 build. There is such a difference:

0x00062228:   DW_TAG_structure_type
                DW_AT_calling_convention        (DW_CC_pass_by_value)
                DW_AT_name      ("__va_list_tag")
                DW_AT_byte_size (0x18)
                DW_AT_decl_file ("/home/maskray/llvm/clang/tools/driver/driver.cpp")
                DW_AT_decl_line (23)

driver.cpp:23 is a #include. So this looks strange. The DW_AT_decl_file/DW_AT_decl_line attributes are undesired due to CurLoc getLineNumber.

I don't understand what you're describing - could you describe it in more detail/help me connect the dots?

It sounds like you're saying you removed the CurLoc fallback and then did a stage 2 build of clang and found an example of worse debug info (I'm not sure what file/line this struct was attributed to currently/without the change you were experimenting with) - that would suggest to me that the CurLoc fallback is helping, by providing a better location than the one you've mentioned here?

[aprantl]

I like that this makes getLineNumber behave less "magic". I'm slightly worried that this isn't quite as NFC as think (and the other cases are just not caught by the testsuite). Perhaps you could compile Clang (and a sample of Clang's ObjC(xx) tests) with the patch and diff the dwarfdump output?

[MaskRay]

Add another workaround. stage 2 -DCMAKE_BUILD_TYPE=Debug clang is byte identical.

[MaskRay]

In D94391#2494531, @dblaikie wrote:

In D94391#2494316, @MaskRay wrote:

I removed CurLoc from call sites and tried a stage 2 build. There is such a difference:

0x00062228:   DW_TAG_structure_type
                DW_AT_calling_convention        (DW_CC_pass_by_value)
                DW_AT_name      ("__va_list_tag")
                DW_AT_byte_size (0x18)
                DW_AT_decl_file ("/home/maskray/llvm/clang/tools/driver/driver.cpp")
                DW_AT_decl_line (23)

driver.cpp:23 is a #include. So this looks strange. The DW_AT_decl_file/DW_AT_decl_line attributes are undesired due to CurLoc getLineNumber.

I don't understand what you're describing - could you describe it in more detail/help me connect the dots?

The ObjC example debug-info-blocks.m is: CurLoc has been advanced to the closing brace. Then getLineNumber is called on ImplicitVarParameter and the code attaches the location of } to this implicit parameter which misses SourceLocation. The location is a bit arbitrary - perhaps the location of ^{ is better.

A worse example is that CurLoc moves further away and getLineNumber is used to attach the line information to a far-away structure. This may potentially unintentionally apply to future AST nodes.

The DW_AT_decl_file __va_list_tag example is about attaching the line of a #include to some declaration deep in the included hierarchy.

It sounds like you're saying you removed the CurLoc fallback and then did a stage 2 build of clang and found an example of worse debug info (I'm not sure what file/line this struct was attributed to currently/without the change you were experimenting with) - that would suggest to me that the CurLoc fallback is helping, by providing a better location than the one you've mentioned here?

Clarified the description.

[dblaikie]

I'll leave this one to @aprantl

[MaskRay]

In D94391#2495056, @dblaikie wrote:

I'll leave this one to @aprantl

@aprantl :)

[MaskRay]

In D94391#2507144, @MaskRay wrote:

In D94391#2495056, @dblaikie wrote:

I'll leave this one to @aprantl

@aprantl :)

:)

clang/lib/CodeGen/CGDebugInfo.cpp

std::string CGDebugInfo::remapDIPath(StringRef Path) const {
   SmallString<256> P = Path;
   for (const auto &Entry : DebugPrefixMap)
     if (llvm::sys::path::replace_path_prefix(P, Entry.first, Entry.second))
       break;
   return P.str().str();
 }
 
 unsigned CGDebugInfo::getLineNumber(SourceLocation Loc) {
-  if (Loc.isInvalid() && CurLoc.isInvalid())
+  if (Loc.isInvalid())
     return 0;
   SourceManager &SM = CGM.getContext().getSourceManager();
-  PresumedLoc PLoc = SM.getPresumedLoc(Loc.isValid() ? Loc : CurLoc);
-  return PLoc.isValid() ? PLoc.getLine() : 0;
+  return SM.getPresumedLoc(Loc).getLine();
 }
 
 unsigned CGDebugInfo::getColumnNumber(SourceLocation Loc, bool Force) {
   // We may not want column information at all.
   if (!Force && !CGM.getCodeGenOpts().DebugColumnInfo)
     return 0;
 
   // If the location is invalid then use the current column.

 
 llvm::DICompositeType *
 CGDebugInfo::getOrCreateRecordFwdDecl(const RecordType *Ty,
                                       llvm::DIScope *Ctx) {
   const RecordDecl *RD = Ty->getDecl();
   if (llvm::DIType *T = getTypeOrNull(CGM.getContext().getRecordType(RD)))
     return cast<llvm::DICompositeType>(T);
   llvm::DIFile *DefUnit = getOrCreateFile(RD->getLocation());
-  unsigned Line = getLineNumber(RD->getLocation());
+  const unsigned Line =
+      getLineNumber(RD->getLocation().isValid() ? RD->getLocation() : CurLoc);
   StringRef RDName = getClassName(RD);
 
   uint64_t Size = 0;
   uint32_t Align = 0;
 
   const RecordDecl *D = RD->getDefinition();
   if (D && D->isCompleteDefinition())
     Size = CGM.getContext().getTypeSize(Ty);

 CGDebugInfo::createFieldType(StringRef name, QualType type, SourceLocation loc,
                              AccessSpecifier AS, uint64_t offsetInBits,
                              uint32_t AlignInBits, llvm::DIFile *tunit,
                              llvm::DIScope *scope, const RecordDecl *RD) {
   llvm::DIType *debugType = getOrCreateType(type, tunit);
 
   // Get the location for the field.
   llvm::DIFile *file = getOrCreateFile(loc);
-  unsigned line = getLineNumber(loc);
+  const unsigned line = getLineNumber(loc.isValid() ? loc : CurLoc);
 
   uint64_t SizeInBits = 0;
   auto Align = AlignInBits;
   if (!type->isIncompleteArrayType()) {
     TypeInfo TI = CGM.getContext().getTypeInfo(type);
     SizeInBits = TI.Width;
     if (!Align)
       Align = getTypeAlignIfRequired(type, CGM.getContext());

 }
 
 // TODO: Currently used for context chains when limiting debug info.
 llvm::DICompositeType *CGDebugInfo::CreateLimitedType(const RecordType *Ty) {
   RecordDecl *RD = Ty->getDecl();
 
   // Get overall information about the record type for the debug info.
   llvm::DIFile *DefUnit = getOrCreateFile(RD->getLocation());
-  unsigned Line = getLineNumber(RD->getLocation());
+  const unsigned Line =
+      getLineNumber(RD->getLocation().isValid() ? RD->getLocation() : CurLoc);
   StringRef RDName = getClassName(RD);
 
   llvm::DIScope *RDContext = getDeclContextDescriptor(RD);
 
   // If we ended up creating the type during the context chain construction,
   // just return that.
   auto *T = cast_or_null<llvm::DICompositeType>(
       getTypeOrNull(CGM.getContext().getRecordType(RD)));
if (Fn->hasLocalLinkage())
     SPFlags |= llvm::DISubprogram::SPFlagLocalToUnit;
   if (CGM.getLangOpts().Optimize)
     SPFlags |= llvm::DISubprogram::SPFlagOptimized;
 
   llvm::DINode::DIFlags FlagsForDef = Flags | getCallSiteRelatedAttrs();
   llvm::DISubprogram::DISPFlags SPFlagsForDef =
       SPFlags | llvm::DISubprogram::SPFlagDefinition;
 
-  unsigned LineNo = getLineNumber(Loc);
+  const unsigned LineNo = getLineNumber(Loc.isValid() ? Loc : CurLoc);
   unsigned ScopeLine = getLineNumber(ScopeLoc);
   llvm::DISubroutineType *DIFnType = getOrCreateFunctionType(D, FnType, Unit);
   llvm::DISubprogram *Decl = nullptr;
   if (D)
     Decl = isa<ObjCMethodDecl>(D)
                ? getObjCMethodDeclaration(D, DIFnType, LineNo, Flags, SPFlags)
                : getFunctionDeclaration(D);
 
void CGDebugInfo::EmitDeclareOfBlockDeclRefVariable(
 
   // Self is passed along as an implicit non-arg variable in a
   // block. Mark it as the object pointer.
   if (const auto *IPD = dyn_cast<ImplicitParamDecl>(VD))
     if (IPD->getParameterKind() == ImplicitParamDecl::ObjCSelf)
       Ty = CreateSelfType(VD->getType(), Ty);
 
   // Get location information.
-  unsigned Line = getLineNumber(VD->getLocation());
+  const unsigned Line =
+      getLineNumber(VD->getLocation().isValid() ? VD->getLocation() : CurLoc);
   unsigned Column = getColumnNumber(VD->getLocation());
 
   const llvm::DataLayout &target = CGM.getDataLayout();
 
   CharUnits offset = CharUnits::fromQuantity(
       target.getStructLayout(blockInfo.StructureType)
           ->getElementOffset(blockInfo.getCapture(VD).getIndex()));
 

 
 void CGDebugInfo::EmitUsingDirective(const UsingDirectiveDecl &UD) {
   if (!CGM.getCodeGenOpts().hasReducedDebugInfo())
     return;
   const NamespaceDecl *NSDecl = UD.getNominatedNamespace();
   if (!NSDecl->isAnonymousNamespace() ||
       CGM.getCodeGenOpts().DebugExplicitImport) {
     auto Loc = UD.getLocation();
+    if (!Loc.isValid())
+      Loc = CurLoc;
     DBuilder.createImportedModule(
         getCurrentContextDescriptor(cast<Decl>(UD.getDeclContext())),
         getOrCreateNamespace(NSDecl), getOrCreateFile(Loc), getLineNumber(Loc));
   }
 }
 
 void CGDebugInfo::EmitUsingDecl(const UsingDecl &UD) {
   if (!CGM.getCodeGenOpts().hasReducedDebugInfo())