Page MenuHomePhabricator

[Reproducers] Tool to insert SBReproducer macros
ClosedPublic

Authored by JDevlieghere on Jan 16 2019, 6:27 PM.

Details

Summary

Simple tool to automate/simplify the workflow of inserting SB_RECORD and SB_REGSITER macros.

Because the tool won't be part of the build process I don't wan't to over-complicate it. Currently it prints a list of SB_REGISTER macros followed by the source file with the SB_RECORD macro's inserted. This means you'd have to copy over the SB_REGISTER macros manually per source file.

See example output here: https://reviews.llvm.org/P8125

Diff Detail

Repository
rLLDB LLDB

Event Timeline

JDevlieghere created this revision.Jan 16 2019, 6:27 PM
  • Update files in place.
  • Check for existing macros.
  • Skip unsupported signatures (function pointers and void*)
JDevlieghere retitled this revision from [WIP] Tool to insert SBReproducer macros to [Reproducers] Tool to insert SBReproducer macros.

Fixed printing of booleans; the tool would show _Bool instead of bool.

I ran this on a few files and it appears to work with the prototype.

labath added a subscriber: labath.Jan 28 2019, 12:04 AM

Is this ready for review now?

tools/sbrepro/SBRepro.cpp
98 ↗(On Diff #183584)

MacroMacro?

JDevlieghere added reviewers: labath, davide, friss.

Fix MacroMacro and other code cleanups.

labath added inline comments.Jan 29 2019, 3:51 AM
tools/sbrepro/SBRepro.cpp
22–31 ↗(On Diff #183888)

There's a join in ADT/StringExtras.h which should do what you need here.

194–197 ↗(On Diff #183888)

What exactly does this skip? Would e.g. a constructor with an empty body still be considered for macro-ization?

  • Use join from StringExtras.
  • Rename Skip to ShoupdSkip and document its cases.

So, if I understand this correctly, a constructor like SBFoo::SBFoo(){} will not be considered for macroization because it's empty? That doesn't sound right to me. Even an empty constructor has a side effect of constructing the object, which is something that I would expect to see recorded in the trace file. Is there some reason why it is hard/impossible to insert macros into these kinds of functions? If so, it may be worth just warning the user about their existence.

A function with an empty body probably doesn't do anything interesting, so it may be possible to skip recording those, but I think it would make sense to record them nonetheless.

So, if I understand this correctly, a constructor like SBFoo::SBFoo(){} will not be considered for macroization because it's empty? That doesn't sound right to me. Even an empty constructor has a side effect of constructing the object, which is something that I would expect to see recorded in the trace file. Is there some reason why it is hard/impossible to insert macros into these kinds of functions? If so, it may be worth just warning the user about their existence.

A function with an empty body probably doesn't do anything interesting, so it may be possible to skip recording those, but I think it would make sense to record them nonetheless.

Ugh, I was confusing empty bodies with methods without a body (e.g. declarations). Empty bodies are definitely handled! I'll update the comment.

labath accepted this revision.Jan 30 2019, 11:00 AM

ok, looks good then

This revision is now accepted and ready to land.Jan 30 2019, 11:00 AM
  • Skip decls that have a canonical decl in the implementation file.

It doesn't really make sense to land this without the SBReproducer framework, so I'll leave it open until that gets accepted as well.

  • Use new LLDB_ prefix
  • Insert two newlines at the end of the macro, so clang-format fixes it up consistently.
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald Transcript