This is an archive of the discontinued LLVM Phabricator instance.

[-gmodules] Let LLDB log a warning if the Clang module hash mismatches.
ClosedPublic

Authored by aprantl on Nov 14 2019, 2:01 PM.

Details

Summary

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.

Diff Detail

Event Timeline

aprantl created this revision.Nov 14 2019, 2:01 PM
jingham accepted this revision.Nov 14 2019, 6:35 PM

LGTM

lldb/source/Host/common/Host.cpp
297

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?

This revision is now accepted and ready to land.Nov 14 2019, 6:35 PM
labath added a subscriber: labath.Nov 15 2019, 1:50 AM
labath added inline comments.
lldb/include/lldb/Symbol/SymbolFile.h
283–285 ↗(On Diff #229400)

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...

lldb/source/Host/common/Host.cpp
301

Are you sure it's legal to recycle the va_list this way? Should you maybe re-initialize it?

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
1570

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...

1579

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.

1724–1725

Maybe you could have GetDWOId return Optional<uint>. That way, the FIXME will be inside that function, and not in all of it's callers.

aprantl marked 3 inline comments as done.Nov 15 2019, 8:56 AM
aprantl added inline comments.
lldb/include/lldb/Symbol/SymbolFile.h
283–285 ↗(On Diff #229400)

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.

labath added inline comments.Nov 15 2019, 9:18 AM
lldb/include/lldb/Symbol/SymbolFile.h
283–285 ↗(On Diff #229400)

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<SymbolFileDWARF*>(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.
aprantl marked 2 inline comments as done.Nov 15 2019, 9:29 AM
aprantl added inline comments.
lldb/source/Host/common/Host.cpp
297

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?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptNov 15 2019, 11:53 AM