Page MenuHomePhabricator

Set symbol types for function symbols loaded from PE/COFF
ClosedPublic

Authored by amccarth on Jan 25 2016, 4:16 PM.

Details

Summary

This fixes the regression of several tests on Windows after rL258621.

The root problem is that ObjectFilePECOFF was not setting type information for the symbols, and the new CL rejects symbols without type information, breaking functionality like thread step-over.

The fix sets the type information for functions (and creates a TODO for other types).

Along the way, I fixed some typos and formatting that made the code I was debugging harder to understand.

In the long run, we should consider replacing most of ObjectFilePECOFF with the COFF parsing code from LLVM.

Diff Detail

Event Timeline

amccarth updated this revision to Diff 45925.Jan 25 2016, 4:16 PM
amccarth retitled this revision from to Set symbol types for function symbols loaded from PE/COFF.
amccarth updated this object.
amccarth added reviewers: zturner, clayborg.
amccarth added a subscriber: lldb-commits.
clayborg accepted this revision.Jan 25 2016, 4:25 PM
clayborg edited edge metadata.

Looks fine.

This revision is now accepted and ready to land.Jan 25 2016, 4:25 PM
zturner added inline comments.Jan 25 2016, 4:28 PM
include/lldb/Core/RangeMap.h
1230

If you're making cleanup changes anyway, how about a ranged based for.

include/lldb/Symbol/Symtab.h
84

Did you use clang-format? If you had this would have a return type on a new line (although obviously you don't want that here since it woudl be inconsistent with the rest of the file).

I guess you would need to clang-format it, then don't take the change to this line. (Which maybe is exactly what you did, just checking)

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
183

Instead of returning eSymbolTypeInvalid here, how about eSymbolTypeData? If you look in llvm/Support/COFF.h all of the non function types are data.

amccarth marked 2 inline comments as done.Jan 25 2016, 4:41 PM
amccarth added inline comments.
include/lldb/Core/RangeMap.h
1230

I had thought about it, especially since it violates the LLVM style guide to call end() on every iteration, but I was trying not to venture too far from the point of the patch. Done now.

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
183

I'm not sure about that. Some of the symbols are clearly sections (.text, etc.). Those have 0 for the COFF type, which COFF.h says means "No type information or unknown base type." If .text has a "valid" symbol type, then it will be found (instead of, say, "_main"), and I'm not sure if the unwinding/stepping would work right.

zturner added inline comments.Jan 25 2016, 4:45 PM
source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
183

How about:

if (coff_symbol_type == 0)
    return lldb::eSymbolTypeInvalid;
if (coff_symbol_type >> llvm::COFF::SCT_COMPLEX_TYPE_SHIFT) == llvm::COFF::IMAGE_SYM_DTYPE_FUNCTION)
    return lldb::eSymbolTypeCode;
return lldb::eSymbolTypeData;

I feel like we can at least do a little better than always returning eSymbolTypeInvalid if it's not a function without too much work.

amccarth updated this revision to Diff 45933.Jan 25 2016, 4:46 PM
amccarth edited edge metadata.
amccarth marked an inline comment as done.

Mostly clang-format.

amccarth added inline comments.Jan 25 2016, 4:59 PM
source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
197

After discussion with a COFF expert, it seems wise to postpone trying to do anything more with this type mapping.

This revision was automatically updated to reflect the committed changes.