This is an archive of the discontinued LLVM Phabricator instance.

GSYM: add encoding and decoding to FunctionInfo
ClosedPublic

Authored by clayborg on Sep 12 2019, 9:33 AM.

Details

Summary

This is another patch that originated from the full GSYM patch https://reviews.llvm.org/D53379.

This patch adds encoding and decoding of the FunctionInfo objects along with full error handling and tests. Full details of the FunctionInfo encoding format appear in the FunctionInfo.h header file.

Diff Detail

Event Timeline

clayborg created this revision.Sep 12 2019, 9:33 AM

Looks mostly good.

lib/DebugInfo/GSYM/FunctionInfo.cpp
6–7

wrong license!

32

Thanks, I like that interface!

clayborg updated this revision to Diff 219998.Sep 12 2019, 2:23 PM
  • Updates license on FunctionInfo.cpp
aprantl accepted this revision.Sep 12 2019, 3:00 PM

minor nits inline

lib/DebugInfo/GSYM/FunctionInfo.cpp
18

this should be ///; ///< is for inline comments .

100

this seems pointless. I would just write 4.

This revision is now accepted and ready to land.Sep 12 2019, 3:00 PM
clayborg updated this revision to Diff 220012.Sep 12 2019, 3:06 PM
  • Fix comment from /< to /
  • change "sizeof(uint32_t)" to "4"
MaskRay added inline comments.Sep 12 2019, 5:49 PM
unittests/DebugInfo/GSYM/GSYMTest.cpp
28

.operator bool() may be an awkward way to call the overloaded operator.

bool(Err)

243

bool(ExpectedOffset) or !ExpectedOffset (inversed)

clayborg updated this revision to Diff 220139.Sep 13 2019, 10:36 AM
  • Fix two operator bool calls to use "bool(<object>)" instead of "<object>.operator bool()"
clayborg marked 2 inline comments as done.Sep 13 2019, 10:37 AM
MaskRay added inline comments.Sep 13 2019, 6:19 PM
lib/DebugInfo/GSYM/FunctionInfo.cpp
20

If you want to ensure the underlying type is uint32_t, use

enum InfoType : uint32_t

instead of 0u 1u 2u.

clayborg updated this revision to Diff 220341.Sep 16 2019, 7:42 AM
  • Change "enum InfoType" to "enum InfoType : uint32_t"

Any more issues to resolve or does this look good now?

MaskRay accepted this revision.Sep 17 2019, 8:58 AM
clayborg closed this revision.Sep 17 2019, 9:14 AM

$ svn commit
Sending include/llvm/DebugInfo/GSYM/FunctionInfo.h
Sending lib/DebugInfo/GSYM/FunctionInfo.cpp
Sending unittests/DebugInfo/GSYM/GSYMTest.cpp
Transmitting file data ...done
Committing transaction...
Committed revision 372135.

So it seems that "constexpr" not needing to be captured is not consistent across all builds. The attempt to fix with 372144 then caused some buildbots that have warnings as errors to fail since they would warn that you shouldn't capture a constexpr and that became an error. Windows buildbots require the capture to be there or they fail.

r372144 | rksimon | 2019-09-17 10:24:55 -0700 (Tue, 17 Sep 2019) | 1 line

Fix MSVC lambda capture warnings. NFCI.

Solution? Avoid lambdas to keep buildbots happy with:

$ svn commit
Sending        unittests/DebugInfo/GSYM/GSYMTest.cpp
Transmitting file data .done
Committing transaction...
Committed revision 372179.