[Lldb-commits] [PATCH] D70272: [-gmodules] Let LLDB log a warning if the Clang module hash mismatches.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG1cbe0038944a: [-gmodules] Let LLDB log a warning if the
Clang module hash mismatches. (authored by aprantl).
Herald added a project: LLDB.
Changed prior to commit:
https://reviews.llvm.org/D70272?vs=229400=229611#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70272/new/
https://reviews.llvm.org/D70272
Files:
lldb/include/lldb/Utility/Log.h
lldb/packages/Python/lldbsuite/test/lang/objc/modules-hash-mismatch/Makefile
lldb/packages/Python/lldbsuite/test/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py
lldb/packages/Python/lldbsuite/test/lang/objc/modules-hash-mismatch/main.m
lldb/packages/Python/lldbsuite/test/lang/objc/modules-hash-mismatch/other.m
lldb/packages/Python/lldbsuite/test/make/Makefile.rules
lldb/source/Host/common/Host.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -300,6 +300,9 @@
virtual llvm::Optional GetDwoNum() { return llvm::None; }
+ /// If this is a DWARF object with a single CU, return its DW_AT_dwo_id.
+ llvm::Optional GetDWOId();
+
static bool
DIEInDeclContext(const lldb_private::CompilerDeclContext *parent_decl_ctx,
const DWARFDIE );
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1555,6 +1555,40 @@
return DWARFDIE();
}
+/// Return the DW_AT_(GNU_)dwo_name.
+static const char *GetDWOName(DWARFCompileUnit _cu,
+ const DWARFDebugInfoEntry _die) {
+ const char *dwo_name =
+ cu_die.GetAttributeValueAsString(_cu, DW_AT_GNU_dwo_name, nullptr);
+ if (!dwo_name)
+dwo_name =
+cu_die.GetAttributeValueAsString(_cu, DW_AT_dwo_name, nullptr);
+ return dwo_name;
+}
+
+/// Return the DW_AT_(GNU_)dwo_id.
+/// FIXME: Technically 0 is a valid hash.
+static uint64_t GetDWOId(DWARFCompileUnit _cu,
+ const DWARFDebugInfoEntry _die) {
+ uint64_t dwo_id =
+ cu_die.GetAttributeValueAsUnsigned(_cu, DW_AT_GNU_dwo_id, 0);
+ if (!dwo_id)
+dwo_id = cu_die.GetAttributeValueAsUnsigned(_cu, DW_AT_dwo_id, 0);
+ return dwo_id;
+}
+
+llvm::Optional SymbolFileDWARF::GetDWOId() {
+ if (GetNumCompileUnits() == 1) {
+if (auto comp_unit = GetCompileUnitAtIndex(0))
+ if (DWARFCompileUnit *cu = llvm::dyn_cast_or_null(
+ GetDWARFCompileUnit(comp_unit.get(
+if (DWARFDebugInfoEntry *cu_die = cu->DIE().GetDIE())
+ if (uint64_t dwo_id = ::GetDWOId(*cu, *cu_die))
+return dwo_id;
+ }
+ return {};
+}
+
std::unique_ptr
SymbolFileDWARF::GetDwoSymbolFileForCompileUnit(
DWARFUnit , const DWARFDebugInfoEntry _die) {
@@ -1571,15 +1605,13 @@
if (!dwarf_cu)
return nullptr;
- const char *dwo_name =
- cu_die.GetAttributeValueAsString(dwarf_cu, DW_AT_GNU_dwo_name, nullptr);
+ const char *dwo_name = GetDWOName(*dwarf_cu, cu_die);
if (!dwo_name)
return nullptr;
SymbolFileDWARFDwp *dwp_symfile = GetDwpSymbolFile();
if (dwp_symfile) {
-uint64_t dwo_id =
-cu_die.GetAttributeValueAsUnsigned(dwarf_cu, DW_AT_GNU_dwo_id, 0);
+uint64_t dwo_id = ::GetDWOId(*dwarf_cu, cu_die);
std::unique_ptr dwo_symfile =
dwp_symfile->GetSymbolFileForDwoId(*dwarf_cu, dwo_id);
if (dwo_symfile)
@@ -1619,15 +1651,18 @@
if (m_fetched_external_modules)
return;
m_fetched_external_modules = true;
-
DWARFDebugInfo *debug_info = DebugInfo();
// Follow DWO skeleton unit breadcrumbs.
const uint32_t num_compile_units = GetNumCompileUnits();
for (uint32_t cu_idx = 0; cu_idx < num_compile_units; ++cu_idx) {
-DWARFUnit *dwarf_cu = debug_info->GetUnitAtIndex(cu_idx);
+auto *dwarf_cu =
+llvm::dyn_cast(debug_info->GetUnitAtIndex(cu_idx));
+if (!dwarf_cu)
+ continue;
+
const DWARFBaseDIE die = dwarf_cu->GetUnitDIEOnly();
-if (!die || die.HasChildren())
+if (!die || die.HasChildren() || !die.GetDIE())
continue;
const char *name = die.GetAttributeValueAsString(DW_AT_name, nullptr);
@@ -1639,10 +1674,7 @@
if (module_sp)
continue;
-const char *dwo_path =
-die.GetAttributeValueAsString(DW_AT_GNU_dwo_name, nullptr);
-if (!dwo_path)
- dwo_path = die.GetAttributeValueAsString(DW_AT_dwo_name, nullptr);
+const char *dwo_path = GetDWOName(*dwarf_cu, *die.GetDIE());
if (!dwo_path)
continue;
@@ -1685,12 +1717,35 @@
Re: [Lldb-commits] [PATCH] D70272: [-gmodules] Let LLDB log a warning if the Clang module hash mismatches.
I'm not sure that's right. LLDB_LOG_OPTION_VERBOSE is independent of the log channel, it's a basic feature of Log.h/cpp. You test it with Log::GetVerbose. I know it works with the lldb channel because I hide a bunch of extra logging in the step & expr logs behind GetVerbose() == true. Jim > On Nov 15, 2019, at 9:29 AM, Adrian Prantl via Phabricator > wrote: > > aprantl marked 2 inline comments as done. > aprantl added inline comments. > > > > Comment at: lldb/source/Host/common/Host.cpp:300 > va_end(args); > + > + // Log to log channel. This allows testcases to grep for log output. > > jingham wrote: >> On macOS, SystemLog vsprintf's to stderr. So you probably don't want to put >> this out always. Maybe since you don't know where SystemLog is going to >> print things, it would be better to only output this if the log channel is >> set to verbose. That would still allow you to use it in tests, but wouldn't >> introduce any new output in the normal case? > Turns out LIBLLDB has no VERBOSE channel, and I can't add one because all 32 > bits are already defined as channels. In the end this is probably not too bad > — how many users are running with the host log enabled? > > What do you think about making it either/or? It goes to the syslog by default > and to the host log if it is enabled? > > > CHANGES SINCE LAST ACTION > https://reviews.llvm.org/D70272/new/ > > https://reviews.llvm.org/D70272 > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D70272: [-gmodules] Let LLDB log a warning if the Clang module hash mismatches.
aprantl marked 2 inline comments as done. aprantl added inline comments. Comment at: lldb/source/Host/common/Host.cpp:300 va_end(args); + + // Log to log channel. This allows testcases to grep for log output. jingham wrote: > On macOS, SystemLog vsprintf's to stderr. So you probably don't want to put > this out always. Maybe since you don't know where SystemLog is going to > print things, it would be better to only output this if the log channel is > set to verbose. That would still allow you to use it in tests, but wouldn't > introduce any new output in the normal case? Turns out LIBLLDB has no VERBOSE channel, and I can't add one because all 32 bits are already defined as channels. In the end this is probably not too bad — how many users are running with the host log enabled? What do you think about making it either/or? It goes to the syslog by default and to the host log if it is enabled? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70272/new/ https://reviews.llvm.org/D70272 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D70272: [-gmodules] Let LLDB log a warning if the Clang module hash mismatches.
labath added inline comments.
Comment at: lldb/include/lldb/Symbol/SymbolFile.h:283-285
+ /// Return a hash for this symbol file, such as a dwo_id.
+ virtual llvm::Optional GetSymbolHash() { return {}; }
+
aprantl wrote:
> labath wrote:
> > Can we remove this and put a cast in
> > `SymbolFileDWARF::UpdateExternalModuleListIfNeeded` instead? It looks like
> > that function should check that it has really found a dwarf file (and not a
> > pdb for instance) anyway...
> > Can we remove this and put a cast in
> > SymbolFileDWARF::UpdateExternalModuleListIfNeeded instead?
>
> We don't have a mechanism to dynamic-cast between SymbolFiles at the moment
> and I think it's difficult to add one, since it's a base class of an
> open-ended plugin hierarchy. We could require all plugins to register in a
> global enum if we don't care about extensibility. I also thought about each
> class identifying itself via a string that is its class name, but then we
> couldn't implement casting to a base-class easily.
>
> If you have any idea for how to do this elegantly I'm more than happy to
> implement it.
>
> > It looks like that function should check that it has really found a dwarf
> > file (and not a pdb for instance) anyway...
>
> Technically, yes, but you'd need DWARF that encodes the path of a PDB to get
> into that situation, so I'm not super concerned about this.
> If you have any idea for how to do this elegantly I'm more than happy to
> implement it.
I know at least of two ways of doing that:
- the "old" way. This is pretty similar to your string approach and involves
doing something like:
```
if (symfile->GetPluginName() == SymbolFileDWARF::GetPluginNameStatic())
static_cast(symfile)->stuff()
```
and probably doesn't require any extra coding (the presence of
SymbolFileDWARFDebugMap makes things a bit complicated, but I don't think you
need that here(?))
- for the new way, you can look at how we've implemented open-ended casting
hierarchies in other plugins (last example here: D70070). This works with
llvm::dyn_cast and everything. Not all plugins support that yet, but it is a
direction we're going right now.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70272/new/
https://reviews.llvm.org/D70272
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D70272: [-gmodules] Let LLDB log a warning if the Clang module hash mismatches.
aprantl marked 3 inline comments as done.
aprantl added inline comments.
Comment at: lldb/include/lldb/Symbol/SymbolFile.h:283-285
+ /// Return a hash for this symbol file, such as a dwo_id.
+ virtual llvm::Optional GetSymbolHash() { return {}; }
+
labath wrote:
> Can we remove this and put a cast in
> `SymbolFileDWARF::UpdateExternalModuleListIfNeeded` instead? It looks like
> that function should check that it has really found a dwarf file (and not a
> pdb for instance) anyway...
> Can we remove this and put a cast in
> SymbolFileDWARF::UpdateExternalModuleListIfNeeded instead?
We don't have a mechanism to dynamic-cast between SymbolFiles at the moment and
I think it's difficult to add one, since it's a base class of an open-ended
plugin hierarchy. We could require all plugins to register in a global enum if
we don't care about extensibility. I also thought about each class identifying
itself via a string that is its class name, but then we couldn't implement
casting to a base-class easily.
If you have any idea for how to do this elegantly I'm more than happy to
implement it.
> It looks like that function should check that it has really found a dwarf
> file (and not a pdb for instance) anyway...
Technically, yes, but you'd need DWARF that encodes the path of a PDB to get
into that situation, so I'm not super concerned about this.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70272/new/
https://reviews.llvm.org/D70272
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D70272: [-gmodules] Let LLDB log a warning if the Clang module hash mismatches.
labath added inline comments.
Comment at: lldb/include/lldb/Symbol/SymbolFile.h:283-285
+ /// Return a hash for this symbol file, such as a dwo_id.
+ virtual llvm::Optional GetSymbolHash() { return {}; }
+
Can we remove this and put a cast in
`SymbolFileDWARF::UpdateExternalModuleListIfNeeded` instead? It looks like that
function should check that it has really found a dwarf file (and not a pdb for
instance) anyway...
Comment at: lldb/source/Host/common/Host.cpp:304
+ if (log)
+log->VAPrintf(format, args);
}
Are you sure it's legal to recycle the `va_list` this way? Should you maybe
re-initialize it?
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1570
+static uint64_t GetDWOId(DWARFCompileUnit _cu,
+ DWARFDebugInfoEntry cu_die) {
+ uint64_t dwo_id =
`DWARFDebugInfoEntry` objects assume that they are living in one big vector and
do pointer arithmetic on their `this` pointers. I don't think it's wise to copy
them even if that happens to work in this case...
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1579
+llvm::Optional SymbolFileDWARF::GetSymbolHash() {
+ if (auto comp_unit = GetCompileUnitAtIndex(0)) {
+if (DWARFCompileUnit *cu = llvm::dyn_cast_or_null(
With this implementation, the function will return the dwo id of the first
skeleton unit in the main module in the dwo scenario. I think this is very
unexpected and not very useful. I think this should check that the file
contains a just a single compile unit, at least.
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1724-1725
+
+// Verify the DWO hash.
+// FIXME: Technically "0" is a valid hash.
+uint64_t dwo_id = GetDWOId(*dwarf_cu, *die.GetDIE());
Maybe you could have GetDWOId return Optional. That way, the FIXME will
be inside that function, and not in all of it's callers.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70272/new/
https://reviews.llvm.org/D70272
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D70272: [-gmodules] Let LLDB log a warning if the Clang module hash mismatches.
jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. LGTM Comment at: lldb/source/Host/common/Host.cpp:300 va_end(args); + + // Log to log channel. This allows testcases to grep for log output. On macOS, SystemLog vsprintf's to stderr. So you probably don't want to put this out always. Maybe since you don't know where SystemLog is going to print things, it would be better to only output this if the log channel is set to verbose. That would still allow you to use it in tests, but wouldn't introduce any new output in the normal case? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70272/new/ https://reviews.llvm.org/D70272 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D70272: [-gmodules] Let LLDB log a warning if the Clang module hash mismatches.
aprantl created this revision.
aprantl added reviewers: jingham, JDevlieghere.
Herald added a reviewer: jdoerfert.
This feature is mostly there to aid debugging of Clang module issues, since the
only useful actual the end-user can to is to recompile their program. Dsymutil
prints a similar warning in this case.
https://reviews.llvm.org/D70272
Files:
lldb/include/lldb/Symbol/SymbolFile.h
lldb/include/lldb/Utility/Log.h
lldb/packages/Python/lldbsuite/test/lang/objc/modules-hash-mismatch/Makefile
lldb/packages/Python/lldbsuite/test/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py
lldb/packages/Python/lldbsuite/test/lang/objc/modules-hash-mismatch/main.m
lldb/packages/Python/lldbsuite/test/lang/objc/modules-hash-mismatch/other.m
lldb/packages/Python/lldbsuite/test/make/Makefile.rules
lldb/source/Host/common/Host.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -289,6 +289,9 @@
virtual llvm::Optional GetDwoNum() { return llvm::None; }
+ llvm::Optional GetSymbolHash() override;
+
+
static bool
DIEInDeclContext(const lldb_private::CompilerDeclContext *parent_decl_ctx,
const DWARFDIE );
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1553,6 +1553,39 @@
return DWARFDIE();
}
+/// Return the DW_AT_(GNU_)dwo_name.
+static const char *GetDWOName(DWARFCompileUnit _cu,
+ DWARFDebugInfoEntry cu_die) {
+ const char *dwo_name =
+ cu_die.GetAttributeValueAsString(_cu, DW_AT_GNU_dwo_name, nullptr);
+ if (!dwo_name)
+dwo_name =
+cu_die.GetAttributeValueAsString(_cu, DW_AT_dwo_name, nullptr);
+ return dwo_name;
+}
+
+/// Return the DW_AT_(GNU_)dwo_id.
+/// FIXME: Technically 0 is a valid hash.
+static uint64_t GetDWOId(DWARFCompileUnit _cu,
+ DWARFDebugInfoEntry cu_die) {
+ uint64_t dwo_id =
+ cu_die.GetAttributeValueAsUnsigned(_cu, DW_AT_GNU_dwo_id, 0);
+ if (!dwo_id)
+dwo_id = cu_die.GetAttributeValueAsUnsigned(_cu, DW_AT_dwo_id, 0);
+ return dwo_id;
+}
+
+llvm::Optional SymbolFileDWARF::GetSymbolHash() {
+ if (auto comp_unit = GetCompileUnitAtIndex(0)) {
+if (DWARFCompileUnit *cu = llvm::dyn_cast_or_null(
+GetDWARFCompileUnit(comp_unit.get(
+ if (DWARFDebugInfoEntry *cu_die = cu->DIE().GetDIE())
+if (uint64_t dwo_id = GetDWOId(*cu, *cu_die))
+ return dwo_id;
+ }
+ return {};
+}
+
std::unique_ptr
SymbolFileDWARF::GetDwoSymbolFileForCompileUnit(
DWARFUnit , const DWARFDebugInfoEntry _die) {
@@ -1569,15 +1602,13 @@
if (!dwarf_cu)
return nullptr;
- const char *dwo_name =
- cu_die.GetAttributeValueAsString(dwarf_cu, DW_AT_GNU_dwo_name, nullptr);
+ const char *dwo_name = GetDWOName(*dwarf_cu, cu_die);
if (!dwo_name)
return nullptr;
SymbolFileDWARFDwp *dwp_symfile = GetDwpSymbolFile();
if (dwp_symfile) {
-uint64_t dwo_id =
-cu_die.GetAttributeValueAsUnsigned(dwarf_cu, DW_AT_GNU_dwo_id, 0);
+uint64_t dwo_id = GetDWOId(*dwarf_cu, cu_die);
std::unique_ptr dwo_symfile =
dwp_symfile->GetSymbolFileForDwoId(*dwarf_cu, dwo_id);
if (dwo_symfile)
@@ -1617,15 +1648,18 @@
if (m_fetched_external_modules)
return;
m_fetched_external_modules = true;
-
DWARFDebugInfo *debug_info = DebugInfo();
// Follow DWO skeleton unit breadcrumbs.
const uint32_t num_compile_units = GetNumCompileUnits();
for (uint32_t cu_idx = 0; cu_idx < num_compile_units; ++cu_idx) {
-DWARFUnit *dwarf_cu = debug_info->GetUnitAtIndex(cu_idx);
+auto *dwarf_cu =
+llvm::dyn_cast(debug_info->GetUnitAtIndex(cu_idx));
+if (!dwarf_cu)
+ continue;
+
const DWARFBaseDIE die = dwarf_cu->GetUnitDIEOnly();
-if (!die || die.HasChildren())
+if (!die || die.HasChildren() || !die.GetDIE())
continue;
const char *name = die.GetAttributeValueAsString(DW_AT_name, nullptr);
@@ -1637,10 +1671,7 @@
if (module_sp)
continue;
-const char *dwo_path =
-die.GetAttributeValueAsString(DW_AT_GNU_dwo_name, nullptr);
-if (!dwo_path)
- dwo_path = die.GetAttributeValueAsString(DW_AT_dwo_name, nullptr);
+const char *dwo_path = GetDWOName(*dwarf_cu, *die.GetDIE());
if (!dwo_path)
continue;
@@ -1683,12 +1714,34 @@
GetObjectFile()->GetModule()->ReportWarning(
"0x%8.8x: unable to locate module needed for external types: "
"%s\nerror: %s\nDebugging will be
