[Debuginfo] Allow retrieving source files relative to the compilation directory.

[saugustine]

Summary

Dwarf stores source-file names the three parts:
<compilation_directory><include_directory><filename>

Prior to this change, the code only allowed retrieving either all
three as the absolute path, or just the filename. But many
compile-command lines--especially those in hermetic build systems
don't specify an absolute path, nor just the filename, but rather the
path relative to the compilation directory. This features allows
retrieving them in that style.

[saugustine]

This revision also moves the absolute path logic out of the dwarf-4 only side of the conditional. I believe that was a mistake in an earlier patch, because there is nothing special about dwarf-4 vs dwarf-5 in this regard. Although there are some changes in where it is stored, they aren't relevant to whether it should be appended.

[jhenderson]

This change looks like it's a little outside my area of understanding, so I've added a few of the DebugInfo guys as reviewers.

Where exactly is this new Kind actually created? At the moment, RelativeFilePath looks to be dead. Do you have a follow-up patch for it? Knowing where it is used would point towards the appropriate testing strategy, I think.

[labath]

DWARF5 still says (when describing the directory entries): "Each additional path entry is either a full path name or is relative to the current directory of the compilation.", so I believe this change is correct.

However, I am puzzled by the addition of the RelativeFilePath enum, which is only used in an assertion.

Also, it seems like a test case would be in order here.

[saugustine]

The enum is present only in an assert because as the code is written, it works as, an "other" case. It is something a client would pass, and right now there are no clients inside llvm. I guess I could add one to the symbolizer.

As I mentioned in the original message, I would love to add a test case for this, but I'm not sure where. The rest of the code isn't explicitly tested except by indirectly by way of symbolizer tests. So I guess I can go that route.

What flag would you like? Absolute is the symbolizer default, and "-s" makes it report basenames only.

[dblaikie]

In D73383#1842442, @saugustine wrote:

The enum is present only in an assert because as the code is written, it works as, an "other" case. It is something a client would pass, and right now there are no clients inside llvm. I guess I could add one to the symbolizer.

Yeah, makes sense to me.

As I mentioned in the original message, I would love to add a test case for this, but I'm not sure where. The rest of the code isn't explicitly tested except by indirectly by way of symbolizer tests. So I guess I can go that route.

Right - LLVM has few API level unit tests (though there are some for libDebugInfoDWARF in llvm/unittests/DebugInfo/DWARF/ for instance), most testing is done by exposure through command line tools.

What flag would you like? Absolute is the symbolizer default, and "-s" makes it report basenames only.

Doesn't have to be super brief/punchy if we don't have a normal/common/regular use for it. I'd just go with "-no-comp-dir" for now.

In D73383#1839761, @saugustine wrote:

This revision also moves the absolute path logic out of the dwarf-4 only side of the conditional. I believe that was a mistake in an earlier patch, because there is nothing special about dwarf-4 vs dwarf-5 in this regard. Although there are some changes in where it is stored, they aren't relevant to whether it should be appended.

That might be worthy of a separate patch & test case demonstrating the bug that's being fixed by moving that code out of the conditional.

[saugustine]

In D73383#1842631, @dblaikie wrote:

This revision also moves the absolute path logic out of the dwarf-4 only side of the conditional. I believe that was a mistake in an earlier patch, because there is nothing special about dwarf-4 vs dwarf-5 in this regard. Although there are some changes in where it is stored, they aren't relevant to whether it should be appended.

That might be worthy of a separate patch & test case demonstrating the bug that's being fixed by moving that code out of the conditional.

https://reviews.llvm.org/D73583

The new test case fails without the accompanying fix.

[dblaikie]

In D73383#1845661, @saugustine wrote:

In D73383#1842631, @dblaikie wrote:

This revision also moves the absolute path logic out of the dwarf-4 only side of the conditional. I believe that was a mistake in an earlier patch, because there is nothing special about dwarf-4 vs dwarf-5 in this regard. Although there are some changes in where it is stored, they aren't relevant to whether it should be appended.

That might be worthy of a separate patch & test case demonstrating the bug that's being fixed by moving that code out of the conditional.

https://reviews.llvm.org/D73583

The new test case fails without the accompanying fix.

Great, thanks! So once that's committed you can update this review with more test coverage in that test file?

[labath]

In D73383#1841305, @labath wrote:

Also, it seems like a test case would be in order here.

My bad, I glossed over that part of the patch description. Anyway, I think others can do a better job at reviewing this.

[MaskRay]

This patch needs a rebase after D73583 is committed as 0758ac4e0cfbce67223ce2304e3b4b96c5006010.

Where exactly is this new Kind actually created? At the moment, RelativeFilePath looks to be dead. Do you have a follow-up patch for it? Knowing where it is used would point towards the appropriate testing strategy, I think.

Same question. Where is RelativeFilePath used? We can add a test to unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

[saugustine]

Rebase for split out changes, and add a test.

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

1085

The first half of this check is dead: there's a return false earlier for Kind == FileLineInfoKind::None

I'd also like to see tests using None and Default kinds.

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

53

Rather than needing to create an entire custom prologue, you can use createBasicPrologue to generate a default one, and then just tweak the contents to suit your needs (not forgetting to update the length fields in the process). See ErrorForUnitLengthTooLarge for an example usage.

This will improve readability as it will more clearly show what is important to to this test.

93

Could we use "compiler directory" in the comment please?

96

No need to specify llvm:: here.

99

I think you haven't clang-formatted this?

[saugustine]

Update for comments.

[saugustine]

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

53

Doing this required modifying how basic prologues are created, which adds a few changes in random places. I had proposed D74249 separately, but the reviewer recommended I roll that into this.

[jhenderson]

Aside from the inline comment, looks good to me.

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

53

As noted in D74249, you can avoid folding it in, by extending the checkDefaultPrologue checks.

[dblaikie]

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

53

To avoid more churn in review, etc, I went ahead and committed D74249 with the suggested change to `checkDefaultPrologue``` ( in 0bd48c3d4ee1a94ea3d3b9d89201b23fd83c94d0 ) - so should be able to sync/resolve this patch with that and move forward from there.

[saugustine]

Address upstream comments.

[saugustine]

I think this fixes everything. OK to submit?

[dblaikie]

Sure - looks good :)

[MaskRay]

From what I can tell, DILineInfoSpecifier::FileLineInfoKind::RelativeFilePath can only be set by unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp . Is there a plan to use it in other places of DebugInfo?

llvm/include/llvm/DebugInfo/DIContext.h

 
 /// A DINameKind is passed to name search methods to specify a
 /// preference regarding the type of name resolution the caller wants.
 enum class DINameKind { None, ShortName, LinkageName };
 
 /// Controls which fields of DILineInfo container should be filled
 /// with data.
 struct DILineInfoSpecifier {
-  enum class FileLineInfoKind { None, Default, AbsoluteFilePath };
+  enum class FileLineInfoKind {
+    None,
+    Default,
+    RelativeFilePath,
+    AbsoluteFilePath
+  };
   using FunctionNameKind = DINameKind;
 
   FileLineInfoKind FLIKind;
   FunctionNameKind FNKind;
 
   DILineInfoSpecifier(FileLineInfoKind FLIKind = FileLineInfoKind::Default,
                       FunctionNameKind FNKind = FunctionNameKind::None)
       : FLIKind(FLIKind), FNKind(FNKind) {}

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

bool DWARFDebugLine::Prologue::getFileNameByIndex(
     std::string &Result, sys::path::Style Style) const {
   if (Kind == FileLineInfoKind::None || !hasFileAtIndex(FileIndex))
     return false;
   const FileNameEntry &Entry = getFileNameEntry(FileIndex);
   Optional<const char *> Name = Entry.Name.getAsCString();
   if (!Name)
     return false;
   StringRef FileName = *Name;
-  if (Kind != FileLineInfoKind::AbsoluteFilePath ||
+  if (Kind == FileLineInfoKind::Default ||
       isPathAbsoluteOnWindowsOrPosix(FileName)) {
     Result = std::string(FileName);
     return true;
   }
 
   SmallString<16> FilePath;
   StringRef IncludeDir;
   // Be defensive about the contents of Entry.
   if (getVersion() >= 5) {
     if (Entry.DirIdx < IncludeDirectories.size())
       IncludeDir = IncludeDirectories[Entry.DirIdx].getAsCString().getValue();
   } else {
     if (0 < Entry.DirIdx && Entry.DirIdx <= IncludeDirectories.size())
       IncludeDir =
           IncludeDirectories[Entry.DirIdx - 1].getAsCString().getValue();
   }
-  // We may still need to append compilation directory of compile unit.
-  // We know that FileName is not absolute, the only way to have an
-  // absolute path at this point would be if IncludeDir is absolute.
-  if (!CompDir.empty() && !isPathAbsoluteOnWindowsOrPosix(IncludeDir))
+  // For absolute paths only, include the compilation directory of compile unit.
+  // We know that FileName is not absolute, the only way to have an absolute
+  // path at this point would be if IncludeDir is absolute.
+  if (Kind == FileLineInfoKind::AbsoluteFilePath && !CompDir.empty() &&
+      !isPathAbsoluteOnWindowsOrPosix(IncludeDir))
     sys::path::append(FilePath, Style, CompDir);
 
+  assert((Kind == FileLineInfoKind::AbsoluteFilePath ||
+          Kind == FileLineInfoKind::RelativeFilePath) &&
+         "invalid FileLineInfo Kind");
+
   // sys::path::append skips empty strings.
   sys::path::append(FilePath, Style, IncludeDir, FileName);
   Result = std::string(FilePath.str());
   return true;
 }
 
 bool DWARFDebugLine::LineTable::getFileLineInfoForAddress(
     object::SectionedAddress Address, const char *CompDir,

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

bool setupGenerator(uint16_t Version = 4, uint8_t AddrSize = 8) {
     auto ExpectedGenerator = Generator::create(T, Version);
     if (ExpectedGenerator)
       Gen.reset(ExpectedGenerator->release());
     return true;
   }
 
   void generate() {
     Context = createContext();
     assert(Context != nullptr && "test state is not valid");
     const DWARFObject &Obj = Context->getDWARFObj();
     uint8_t TargetAddrSize = AddressSize == 0 ? 8 : AddressSize;
     LineData = DWARFDataExtractor(
         Obj, Obj.getLineSection(),
         getDefaultTargetTripleForAddrSize(TargetAddrSize).isLittleEndian(),
         AddressSize);
   }
 
DWARFDebugLine::SectionParser setupParser() {
     LT2.addExtendedOpcode(1, DW_LNE_end_sequence, {});
 
     generate();
 
     return DWARFDebugLine::SectionParser(LineData, *Context, CUs, TUs);
   }
 
   void recordRecoverable(Error Err) {
     Recoverable = joinErrors(std::move(Recoverable), std::move(Err));
   }
   void recordUnrecoverable(Error Err) {
     Unrecoverable = joinErrors(std::move(Unrecoverable), std::move(Err));
   }
 
   void checkError(ArrayRef<StringRef> ExpectedMsgs, Error Err) {
     ASSERT_TRUE(Err.operator bool());
     size_t WhichMsg = 0;
     Error Remaining =
         handleErrors(std::move(Err), [&](const ErrorInfoBase &Actual) {
           ASSERT_LT(WhichMsg, ExpectedMsgs.size());
           // Use .str(), because googletest doesn't visualise a StringRef
           // properly.
           EXPECT_EQ(Actual.message(), ExpectedMsgs[WhichMsg++].str());
EXPECT_TRUE(
       << Output;
   EXPECT_TRUE(InOutput("0x0000003a: 09 DW_LNS_fixed_advance_pc (0x0037)\n"))
       << Output;
   EXPECT_TRUE(InOutput("0x0000003d: 0a DW_LNS_set_prologue_end\n")) << Output;
   EXPECT_TRUE(InOutput("0x0000003e: 0b DW_LNS_set_epilogue_begin\n")) << Output;
   EXPECT_TRUE(InOutput("0x0000003f: 0c DW_LNS_set_isa (66)\n")) << Output;
 }
 
+TEST_F(DebugLineBasicFixture, PrintPathsProperly) {
+  if (!setupGenerator(5))
+    return;
+
+  LineTable &LT = Gen->addLineTable();
+  DWARFDebugLine::Prologue P = LT.createBasicPrologue();
+  P.IncludeDirectories.push_back(
+      DWARFFormValue::createFromPValue(DW_FORM_string, "b dir"));
+  P.FileNames.push_back(DWARFDebugLine::FileNameEntry());
+  P.FileNames.back().Name =
+      DWARFFormValue::createFromPValue(DW_FORM_string, "b file");
+  P.FileNames.back().DirIdx = 1;
+  P.PrologueLength += 14;
+  LT.setPrologue(P);
+  generate();
+
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+                                                    nullptr, RecordRecoverable);
+  EXPECT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+  std::string Result;
+  // DWARF 5 stores the compilation directory in two places: the Compilation
+  // Unit and the directory table entry 0, and implementations are free to use
+  // one or the other. This copy serves as the one stored in the CU.
+  StringRef CompDir = "a dir";
+  EXPECT_FALSE(
+      (*ExpectedLineTable)
+          ->Prologue.getFileNameByIndex(
+              1, CompDir, DILineInfoSpecifier::FileLineInfoKind::None, Result));
+  EXPECT_TRUE((*ExpectedLineTable)
+                  ->Prologue.getFileNameByIndex(
+                      1, CompDir,
+                      DILineInfoSpecifier::FileLineInfoKind::Default, Result));
+  EXPECT_STREQ(Result.c_str(), "b file");
+  EXPECT_TRUE((*ExpectedLineTable)
+                  ->Prologue.getFileNameByIndex(
+                      1, CompDir,
+                      DILineInfoSpecifier::FileLineInfoKind::RelativeFilePath,
+                      Result));
+  EXPECT_STREQ(Result.c_str(), "b dir/b file");
+  EXPECT_TRUE((*ExpectedLineTable)
+                  ->Prologue.getFileNameByIndex(
+                      1, CompDir,
+                      DILineInfoSpecifier::FileLineInfoKind::AbsoluteFilePath,
+                      Result));
+  EXPECT_STREQ(Result.c_str(), "a dir/b dir/b file");
+}
+
 } // end anonymous namespace