This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Remove all the 'current_id' logging counters from the lookup code.
ClosedPublic

Authored by teemperor on Feb 21 2020, 1:33 AM.

Details

Summary

We have a lot of code in our lookup code to pass around current_id counters which end up in our logs like this:

AOCTV::FT [234] Found XYZ

This patch removes all of this code because:

  • I'm splitting up all humongous functions, so I need to write more and more boilerplate to pass around these ids.
  • I never saw any similar counters in the LLDB/LLVM code base.
  • They're essentially globals and the last thing we need in LLDB is even more global state.
  • They're not really useful when readings logs. It doesn't help that there isn't just 1 or 2 counters, but 12 (!) unique counters. I always thought that if I see two identical counter values in those brackets it's the same lookup request, but it seems that's only true by accident (and you can't know which of the 12 counters is actually printed without reading the code). The only time I know I can trust the counters is when it's obvious from the log that it's the same counter like in the log below, but then why have the counters in the first place?
LayoutRecordType[28] on (ASTContext*)0x00007FFA1C840200 'scratch ASTContext' for (RecordDecl*)0x00007FFA0AAE8CF0 [name = '__tree']
LRT[28] returned:
LRT[28]   Original = (RecordDecl*)%p
LRT[28]   Size = %lld
LRT[28]   Alignment = %lld
LRT[28]   Fields:
LRT[28]     (FieldDecl*)0x00007FFA1A13B1D0, Name = '__begin_node_', Offset = 0 bits
LRT[28]     (FieldDecl*)0x00007FFA1C08FD30, Name = '__pair1_', Offset = 64 bits
LRT[28]     (FieldDecl*)0x00007FFA1C061210, Name = '__pair3_', Offset = 128 bits
LRT[28]   Bases:

Diff Detail

Event Timeline

teemperor created this revision.Feb 21 2020, 1:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2020, 1:33 AM
teemperor retitled this revision from [lldb] Remove all the`current_id` logging counters from the lookup code. to [lldb] Remove all the 'current_id' logging counters from the lookup code..Feb 21 2020, 1:33 AM

Just a drive-by suggestion: Logging the pointer value of the relevant root object in the callees would provide similar benefit to anyone needing to do log tracing

grandinj removed a subscriber: grandinj.Feb 21 2020, 6:38 AM
shafik accepted this revision.Feb 21 2020, 7:02 AM

I have struggled to understand the real use of those counters, they may have been helpful to someone at one point but the rationale is lost in the sands of time.

LGTM but lets see if someone else has an insight.

This revision is now accepted and ready to land.Feb 21 2020, 7:02 AM
JDevlieghere accepted this revision.Feb 21 2020, 8:17 AM
labath accepted this revision.Feb 23 2020, 11:09 PM

I rarely visit this part of the code, but on the few occasions that I had to debug it, I did not find the ID numbers useful..

This revision was automatically updated to reflect the committed changes.