This is an archive of the discontinued LLVM Phabricator instance.

Refactor 1 symbol record
ClosedPublic

Authored by zturner on May 18 2016, 2:13 PM.

Details

Summary

There are many more, but this just touches 1 record so we can see what the code is going to look like.

There is one potential problem with this patch which exposes a limitation in the underlying design we've chosen for the symbol and type records. dumping symbol records relies on calling a function printRelocatedField which assumes that you have a pointer into the section data so that it can compute the section offset. This isn't true anymore once we're copying bytes out of the input stream and into an in memory structure, so it makes it so we can't use printRelocatedField. However, this particular symbol (ProcSym) does have another field which is the section offset. So maybe we can use that. But I haven't checked whether it's the same as what we think it is yet, nor have I confirmed that this will work for the general case of all other types of symbols.

Diff Detail

Event Timeline

zturner updated this revision to Diff 57677.May 18 2016, 2:13 PM
zturner retitled this revision from to Refactor 1 symbol record.
zturner updated this object.
zturner added a reviewer: rnk.
zturner added a subscriber: llvm-commits.
zturner updated this revision to Diff 57805.May 19 2016, 8:39 AM

Slightly changed the way we do this so that we can still use the relocated field to get the linkage name. This diverges slightly from how we're doing type records, but not by much, and it still allows us to use the generic deserialize methods. But we just store a pointer directly to the layout rather than copying the fields. Who knows, maybe we can eventually adopt this approach for types as well.

Yea originally PublicSymbolStreamIndex was called PSSyms too, so it must be the same correlation.

zturner updated this revision to Diff 57844.May 19 2016, 12:58 PM

This refactors all the symbol records as discussed offline. The code got shorter in some places, but more verbose in others, as now every case of the switch statement has to repeat the same 5-6 lines of code to try the deserialization.

This is just a stepping stone to introducing a symbol visitor however, and once we have this will become much simpler. I didn't want to do too much in one patch though, so that will come in a followup

rnk added inline comments.May 19 2016, 12:59 PM
include/llvm/DebugInfo/CodeView/SymbolRecord.h
95

Should this really modify the Annotations member variable? Maybe it should be a static method that takes an ArrayRef by non-const reference.

Also, it should be in a .cpp file.

zturner added inline comments.May 19 2016, 1:02 PM
include/llvm/DebugInfo/CodeView/SymbolRecord.h
274

Probably not, but that's essentially what the old code did. I can make it so that it doesn't.

zturner updated this revision to Diff 57954.May 20 2016, 10:59 AM

Updated based on rnk's previous comments. Probably overkill, but now you can iterate annotations using ranged base for syntax.

rnk accepted this revision.May 20 2016, 3:48 PM
rnk edited edge metadata.

rubber stamp, looks good =)

tools/llvm-readobj/COFFDumper.cpp
1157–1166

Should this be consistent and use Annotation.Name?

This revision is now accepted and ready to land.May 20 2016, 3:48 PM
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Committed in r270475.