Page MenuHomePhabricator

[Remarks] Add support for linking remarks
ClosedPublic

Authored by thegameg on Oct 17 2019, 2:32 PM.

Details

Summary

Remarks are usually emitted per-TU, and for generating a standalone remark file that can be shipped with the linked binary we need some kind of tool to merge everything together.

The remarks::RemarkLinker class takes care of this and:

  • Deduplicates remarks
  • Filters remarks with no debug location
  • Merges string tables from all the entries

As an output, it provides an iterator range that can be used to serialize the remarks to a file.

Diff Detail

Event Timeline

thegameg created this revision.Oct 17 2019, 2:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2019, 2:32 PM
JDevlieghere added inline comments.Mon, Oct 21, 1:29 PM
llvm/include/llvm/Remarks/Remark.h
119

Why does this not return false?

llvm/include/llvm/Remarks/RemarkLinker.h
34

Should we have an assert here that says that both pointers should be valid?

40

A newline between members (and method below) would really improve readability.

llvm/lib/Remarks/RemarkLinker.cpp
109

No else after return

111

No else after return

thegameg updated this revision to Diff 226348.Thu, Oct 24, 3:41 PM
thegameg marked 4 inline comments as done.
thegameg marked an inline comment as done.
thegameg added inline comments.
llvm/include/llvm/Remarks/Remark.h
119

This way, we order None before a valid Optional<T>. For example, remarks with no debug location will appear first.

llvm/lib/Remarks/RemarkLinker.cpp
111

This one is needed in order to access SectionOrErr. I can remove it from the condition if you think it's nicer.

JDevlieghere accepted this revision.Thu, Oct 31, 9:10 AM

Few small comments inline but otherwise this LGTM

llvm/include/llvm/Remarks/Remark.h
119

Sounds like this is worth a comment.

llvm/include/llvm/Remarks/RemarkLinker.h
93

Do you need both as part of the public API? Who's externally modifying the string table?

llvm/lib/Remarks/RemarkLinker.cpp
109

There's still and else after return Error::success();.

This revision is now accepted and ready to land.Thu, Oct 31, 9:10 AM
This revision was automatically updated to reflect the committed changes.
thegameg marked 4 inline comments as done.