This is an archive of the discontinued LLVM Phabricator instance.

[LLDB] Change formatting to use llvm::formatv
ClosedPublic

Authored by ayermolo on Dec 13 2022, 10:33 AM.

Details

Summary

In preparation for eanbling 64bit support in LLDB switching to use llvm::formatv
instead of format MACROs.

Diff Detail

Event Timeline

ayermolo created this revision.Dec 13 2022, 10:33 AM
Herald added a project: Restricted Project. · View Herald Transcript
ayermolo requested review of this revision.Dec 13 2022, 10:33 AM
Herald added a project: Restricted Project. · View Herald Transcript
JDevlieghere requested changes to this revision.Dec 13 2022, 11:42 AM

I don't really understand the motivation. Can you elaborate on why "enabling 64 bit support" requires this change? I definitely prefer the formatv approach, but wouldn't the PRIx64 should do what you want here?

Also, at a higher level, can we do the same thing as the Log::Format function that takes varargs and uses formavt under the hood?

template <typename... Args>
void Format(llvm::StringRef file, llvm::StringRef function,
            const char *format, Args &&... args) {
  Format(file, function, llvm::formatv(format, std::forward<Args>(args)...));
}
lldb/include/lldb/Core/Module.h
826–847

Let's not keep both.

849

This should be (1) a doxygen comment and (2) using a group. But that's moot if we don't keep the printf-style variants around.

lldb/source/Core/Module.cpp
1169–1174

This is not using msg. Missing strm.PutCString(msg);?

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
15–56

Unrelated change?

This revision now requires changes to proceed.Dec 13 2022, 11:42 AM
ayermolo added a comment.EditedDec 14 2022, 10:45 AM

I don't really understand the motivation. Can you elaborate on why "enabling 64 bit support" requires this change? I definitely prefer the formatv approach, but wouldn't the PRIx64 should do what you want here?

Also, at a higher level, can we do the same thing as the Log::Format function that takes varargs and uses formavt under the hood?

template <typename... Args>
void Format(llvm::StringRef file, llvm::StringRef function,
            const char *format, Args &&... args) {
  Format(file, function, llvm::formatv(format, std::forward<Args>(args)...));
}

This was suggested in https://reviews.llvm.org/D138618 by @labath My understanding to get away from the PRIx32/PRIx64 macros.
We are changing the dw_offset_t to be 64bit, and need to change wherever it's printed to print 64bit number.

Yeah, I'm afraid this is all my fault. I suggested going in this direction, but this isn't exactly how I thought it would turn out. I was expecting that the inferface would be something along the lines of what Jonas posted in his comment. And I was not expecting that you will try to convert every single instance of PRIx32 dwarf offset printing. The majority of the modified lines are the ReportError and ReportWarning statements, so I though we would just be converting those. And the majority of the callers of these functions are coming from DWARF code, so I agree with Jonas that it would be nice to convert all callers of these functions -- but I won't insist on it.

I guess I should have been more explicit about my expectations -- I'm sorry.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
513

You could just change this to print a different delimiting character ([ for instance).

I tried to modify all the places wehre getOffset() is used. Which will later return 64bit instead of 32 bit.

Sorry I guess there was miss communication.
I am not quite clear in what you are suggesting.
The way I did it we now have two distinct interfaces. One for old way that takes in char* and args and applies formatting internally, and a new one which takes in std::string. So all formatting is done on caller side with std::string(llvm::formatv(..)).
Are you suggesting to change implementation of

void ReportError(const char *format, ...)
      __attribute__((format(printf, 2, 3)));

and others to use llvm::formatv under the hood instead?
So caller side will remain mostly the same. Except instead of using printf formating it will be formatvv.
In that case all of call sites will need to change.

I tried to modify all the places wehre getOffset() is used. Which will later return 64bit instead of 32 bit.

Sorry I guess there was miss communication.
I am not quite clear in what you are suggesting.
The way I did it we now have two distinct interfaces. One for old way that takes in char* and args and applies formatting internally, and a new one which takes in std::string. So all formatting is done on caller side with std::string(llvm::formatv(..)).
Are you suggesting to change implementation of

void ReportError(const char *format, ...)
      __attribute__((format(printf, 2, 3)));

and others to use llvm::formatv under the hood instead?

Yes, that's pretty much it.

So caller side will remain mostly the same. Except instead of using printf formating it will be formatvv.
In that case all of call sites will need to change.

Yes, but in the case of ReportError, most of the callers are in DWARF code anyway, so I think it'd make sense to replace all of them (and some of them are already calling formatv, and then passing the string to this function).

If there are too many callers then you can just make a different function (ReportErrorv, or ReportErrorWithFormatv, depending on how verbose you feel), and then just change the callers that we care about.

I tried to modify all the places wehre getOffset() is used. Which will later return 64bit instead of 32 bit.

Sorry I guess there was miss communication.
I am not quite clear in what you are suggesting.
The way I did it we now have two distinct interfaces. One for old way that takes in char* and args and applies formatting internally, and a new one which takes in std::string. So all formatting is done on caller side with std::string(llvm::formatv(..)).
Are you suggesting to change implementation of

void ReportError(const char *format, ...)
      __attribute__((format(printf, 2, 3)));

and others to use llvm::formatv under the hood instead?

Yes, that's pretty much it.

So caller side will remain mostly the same. Except instead of using printf formating it will be formatvv.
In that case all of call sites will need to change.

Yes, but in the case of ReportError, most of the callers are in DWARF code anyway, so I think it'd make sense to replace all of them (and some of them are already calling formatv, and then passing the string to this function).

If there are too many callers then you can just make a different function (ReportErrorv, or ReportErrorWithFormatv, depending on how verbose you feel), and then just change the callers that we care about.

Ah, gotcha. Thanks for elaborating.

ayermolo marked 2 inline comments as done.Dec 19 2022, 4:09 PM
ayermolo added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
15–56

This was recommended in another diff. Although it was related to DIERef.h. I can move it to the second diff, or it's own diff?

ayermolo updated this revision to Diff 484259.Dec 20 2022, 7:00 AM

pulled a trigger and changed all the call sites. I think less confusing then having two sets of APIs. One with printf symantics another with formatv.

ayermolo marked 2 inline comments as done.Dec 20 2022, 7:02 AM
ayermolo updated this revision to Diff 484261.Dec 20 2022, 7:05 AM

removed include that snuck in.

ayermolo updated this revision to Diff 484382.Dec 20 2022, 2:26 PM

missed couple of Status uses.

This is looking much better -- and focused. I still have comments though. :)

lldb/include/lldb/Core/Module.h
832–837

Could you do this one as well? It only has one caller and it would be strange for it to use a different syntax than LogMessage

841

Let's remove this defensive nonsense while we're in here. All of the callers are in this patch, and none of them is passing an empty string or a null pointer.

lldb/include/lldb/Utility/Status.h
66

I don't think you've converted all the callers of this one. Given it's pervasiveness, I think we will need to do some sort of a staged conversion, to ensure call sites get compile errors instead of silent breakage. For now I think it would be fine to have a "named constructor" using formatv (static Status Status::WithFormat(fmt, args...)). Or just leave it out...

lldb/source/Core/Module.cpp
1157–1164

duplicated line

lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp
21–28
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
197

this shouldn't be necessary

lldb/source/Plugins/SymbolFile/DWARF/DWARFTypeUnit.cpp
18–24

same here

lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
585–586

This should be enough (also, I think doing a .str() on the format result would look nicer than a std::string constructor)

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
15–56

Yeah, this looks messy if the clang-format layout is significantly different than the original. Maybe you shouldn't do it here -- or do it as a separate patch (but do keep it in mind for future, particularly when creating new files).

3062

delete empty line

lldb/source/Symbol/DWARFCallFrameInfo.cpp
775–781
ayermolo updated this revision to Diff 484920.Dec 22 2022, 11:59 AM
ayermolo marked 11 inline comments as done.

addressing comments

lldb/include/lldb/Utility/Status.h
66

Yeah you are right, missed a bunch. :/ As an experiment changed Status constructor to take a string and caller side to std::string(llvm::formatv() After 26 files, gave up. Would have exploded this diff. Added a static function that constructs Status using formatv

@labath ping. Is there anything else I need to do?

This looks good to me if we drop the explicit + for right alignment: it's the default and other places in LLDB (and LLVM, at least as far as I'm aware) don't include this unless there's ambiguity.

ayermolo updated this revision to Diff 486719.Jan 5 2023, 5:18 PM

removed +

labath accepted this revision.Jan 9 2023, 4:28 AM

I'm sorry for the delay, I was out for the holidays. Looks now, thanks for your patience.

JDevlieghere accepted this revision.Jan 9 2023, 8:57 AM

LGTM too

This revision is now accepted and ready to land.Jan 9 2023, 8:57 AM
This revision was landed with ongoing or failed builds.Jan 9 2023, 11:30 AM
This revision was automatically updated to reflect the committed changes.

Thanks for reviewing, really appreciate it!

Michael137 added a subscriber: Michael137.EditedJan 12 2023, 3:55 AM

FYI, this is causing buildbot failures: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/49913/execution/node/74/log/

Currently checking what's wrong.

/Users/buildslave/jenkins/workspace/lldb-cmake@2/llvm-project/lldb/test/API/lang/cpp/accelerator-table -p TestCPPAccelerator.py
--
Exit Code: -6

Command Output (stdout):
--
lldb version 16.0.99git (https://github.com/llvm/llvm-project.git revision 706881825b2470c7176e07355b6ab54d8b4c5f95)
  clang revision 706881825b2470c7176e07355b6ab54d8b4c5f95
  llvm revision 706881825b2470c7176e07355b6ab54d8b4c5f95

--
Command Output (stderr):
--
Assertion failed: (Style.empty() && "Invalid integral format style!"), function format, file /Users/buildslave/jenkins/workspace/lldb-cmake@2/llvm-project/llvm/include/llvm/Support/FormatProviders.h, line 147.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	HandleCommand(command = "frame variable inner_d")
frame #39: 0x0000000195adff34 dyld`start + 2248                                                                                                                                                                                                                                                                                                                           
frame #38: 0x0000000100006050 lldb`main(argc=10, argv=0x000000016fdfe8e8) at Driver.cpp:820:26                                                                                                                                                                                                                                                                            
frame #37: 0x000000010000558c lldb`Driver::MainLoop(this=0x000000016fdfe148) at Driver.cpp:577:20                                                                                     
frame #36: 0x000000012e38838c liblldb.16.0.0git.dylib`lldb::SBDebugger::RunCommandInterpreter(this=0x000000016fdfe168, options=0x000000016fdfde48) at SBDebugger.cpp:1274:14          
frame #35: 0x000000012e8abe54 liblldb.16.0.0git.dylib`lldb_private::CommandInterpreter::RunCommandInterpreter(this=0x000000010410cdb0, options=0x0000600000c1d140) at CommandInterpreter.cpp:3364:16
frame #34: 0x000000012e642fbc liblldb.16.0.0git.dylib`lldb_private::Debugger::RunIOHandlers(this=0x0000000104814800) at Debugger.cpp:1006:16                                                                                                                                                                                                                              
frame #33: 0x000000012e698ae4 liblldb.16.0.0git.dylib`lldb_private::IOHandlerEditline::Run(this=0x0000000100a053d8) at IOHandler.cpp:576:22                                           
frame #32: 0x000000012e8aa708 liblldb.16.0.0git.dylib`lldb_private::CommandInterpreter::IOHandlerInputComplete(this=0x000000010410cdb0, io_handler=0x0000000100a053d8, line="v inner_d") at CommandInterpreter.cpp:3110:3
frame #31: 0x000000012e8a5ef0 liblldb.16.0.0git.dylib`lldb_private::CommandInterpreter::HandleCommand(this=0x000000010410cdb0, command_line="v inner_d", lazy_add_to_history=eLazyBoolCalculate, result=0x000000016fdfd8b0) at CommandInterpreter.cpp:2035:14
frame #30: 0x000000012e8d1aec liblldb.16.0.0git.dylib`lldb_private::CommandObjectParsed::Execute(this=0x0000000104833600, args_string="inner_d", result=0x000000016fdfd8b0) at CommandObject.cpp:747:19
frame #29: 0x00000001325039bc liblldb.16.0.0git.dylib`CommandObjectFrameVariable::DoExecute(this=0x0000000104833600, command=0x000000016fdfd278, result=0x000000016fdfd8b0) at CommandObjectFrame.cpp:585:32
frame #28: 0x000000012eab1ff4 liblldb.16.0.0git.dylib`lldb_private::StackFrame::GetValueForVariableExpressionPath(this=0x0000000100a06ca8, var_expr=(Data = "", Length = 0), use_dynamic=eDynamicDontRunTarget, options=49, var_sp=std::__1::shared_ptr<lldb_private::Variable>::element_type @ 0x0000600003b28478 strong=5 weak=2, error=0x000000016fdfce20) at StackFram
e.cpp:619:17                                                                                                                                                                          
frame #27: 0x000000012eab42b8 liblldb.16.0.0git.dylib`lldb_private::StackFrame::GetValueObjectForFrameVariable(this=0x0000000100a06ca8, variable_sp=std::__1::shared_ptr<lldb_private::Variable>::element_type @ 0x0000600003b28478 strong=5 weak=2, use_dynamic=eDynamicDontRunTarget) at StackFrame.cpp:1190:43
frame #26: 0x000000012e76e660 liblldb.16.0.0git.dylib`lldb_private::ValueObject::GetDynamicValue(this=0x000000010189b400, use_dynamic=eDynamicDontRunTarget) at ValueObject.cpp:1849:5 
frame #25: 0x000000012e76e588 liblldb.16.0.0git.dylib`lldb_private::ValueObject::CalculateDynamicValue(this=0x000000010189b400, use_dynamic=eDynamicDontRunTarget) at ValueObject.cpp:1837:29
frame #24: 0x000000012ea5b5d8 liblldb.16.0.0git.dylib`lldb_private::Process::IsPossibleDynamicValue(this=0x000000010289e400, in_value=0x000000010189b400) at Process.cpp:1547:38      
frame #23: 0x000000012e773234 liblldb.16.0.0git.dylib`lldb_private::ValueObject::GetObjectRuntimeLanguage(this=0x000000010189b400) at ValueObject.h:374:12                            
frame #22: 0x000000012e488f7c liblldb.16.0.0git.dylib`lldb_private::ValueObject::GetCompilerType(this=0x000000010189b400) at ValueObject.h:352:43                                     
frame #21: 0x000000012e7669f0 liblldb.16.0.0git.dylib`lldb_private::ValueObject::MaybeCalculateCompleteType(this=0x000000010189b400) at ValueObject.cpp:248:30                        
frame #20: 0x000000012e78f5d8 liblldb.16.0.0git.dylib`lldb_private::ValueObjectVariable::GetCompilerTypeImpl(this=0x000000010189b400) at ValueObjectVariable.cpp:70:35                
frame #19: 0x000000012e9f3ae4 liblldb.16.0.0git.dylib`lldb_private::Variable::GetType(this=0x0000600003b28478) at Variable.cpp:98:31                                                  
frame #18: 0x000000012e9d3320 liblldb.16.0.0git.dylib`lldb_private::SymbolFileType::GetType(this=0x0000600002145e68) at Type.cpp:137:41                                               
frame #17: 0x000000012f3a2d2c liblldb.16.0.0git.dylib`SymbolFileDWARF::ResolveTypeUID(this=0x0000000102825000, type_uid=4294967591) at SymbolFileDWARF.cpp:1498:21                    
frame #16: 0x000000012f368878 liblldb.16.0.0git.dylib`DWARFDIE::ResolveType(this=0x000000016fdfb790) const at DWARFDIE.cpp:351:24                                                     
frame #15: 0x000000012f3a2ed0 liblldb.16.0.0git.dylib`SymbolFileDWARF::ResolveType(this=0x0000000102825000, die=0x000000016fdfb790, assert_not_being_parsed=true, resolve_function_context=false) at SymbolFileDWARF.cpp:1625:18
frame #14: 0x000000012f3a4280 liblldb.16.0.0git.dylib`SymbolFileDWARF::GetTypeForDIE(this=0x0000000102825000, die=0x000000016fdfb790, resolve_function_context=false) at SymbolFileDWARF.cpp:2715:17
frame #13: 0x000000012f3a9278 liblldb.16.0.0git.dylib`SymbolFileDWARF::ParseType(this=0x0000000102825000, sc=0x000000016fdfb598, die=0x000000016fdfb790, type_is_new_ptr=0x0000000000000000) at SymbolFileDWARF.cpp:3123:31
frame #12: 0x000000012f321e00 liblldb.16.0.0git.dylib`DWARFASTParserClang::ParseTypeFromDWARF(this=0x0000600003008480, sc=0x000000016fdfb598, die=0x000000016fdfb790, type_is_new_ptr=0x0000000000000000) at DWARFASTParserClang.cpp:429:42
frame #11: 0x000000012f322520 liblldb.16.0.0git.dylib`void lldb_private::Module::LogMessage<unsigned int, void*, unsigned int, char const*, char const*>(this=0x0000000103029ca8, log=0x0000600003d14008, format="DWARFASTParserClang::ParseTypeFromDWARF (die = {0:x16}, decl_ctx = {1:p} (die {2:16x})) (3:s) name = '{4}')", args=0x000000016fdfb21c, args=0x000000016f
dfb210, args=0x000000016fdfb20c, args=0x000000016fdfb200, args=0x000000016fdfb1f8) at Module.h:830:5
frame #10: 0x000000012e6ec718 liblldb.16.0.0git.dylib`lldb_private::Module::LogMessage(this=0x0000000103029ca8, log=0x0000600003d14008, payload=0x000000016fdfae08) at Module.cpp:1190:34
frame #9: 0x000000012e3e7e8c liblldb.16.0.0git.dylib`llvm::formatv_object_base::str(this=0x000000016fdfae08) const at FormatVariadic.h:111:12                             
frame #8: 0x000000012f6f1d30 liblldb.16.0.0git.dylib`llvm::raw_ostream::operator<<(llvm::formatv_object_base const&)(this=0x000000016fdfac70, Obj=0x000000016fdfae08) at raw_ostream.cpp:348:7
frame #7: 0x000000012f6f1ee0 liblldb.16.0.0git.dylib`llvm::formatv_object_base::format(this=0x000000016fdfae08, S=0x000000016fdfac70) const at FormatVariadic.h:101:13
frame #6: 0x000000012f6f50e8 liblldb.16.0.0git.dylib`llvm::FmtAlign::format(this=0x000000016fdfaad8, S=0x000000016fdfac70, Options=(Data = "16x})) (3:s) name = '{4}')", Length = 3)) at FormatCommon.h:36:15
frame #5: 0x000000012ec1a268 liblldb.16.0.0git.dylib`llvm::detail::provider_format_adapter<unsigned int>::format(this=0x000000016fdfae48, S=0x000000016fdfac70, Options=(Data = "16x})) (3:s) name = '{4}')", Length = 3)) at FormatVariadicDetails.h:40:5
frame #4: 0x000000012ec190c4 liblldb.16.0.0git.dylib`llvm::format_provider<unsigned int, void>::format(V=0x000000016fdfae50, Stream=0x000000016fdfac70, Style=(Data = "x})) (3:s) name = '{4}')", Length = 1)) at FormatProviders.h:147:5    
frame #3: 0x0000000195d44e84 libsystem_c.dylib`__assert_rtn + 272
Michael137 added a comment.EditedJan 12 2023, 3:58 AM

Ah it's just a typoed format (and lack of curly braces around one of the format specifiers)

Fixed in https://reviews.llvm.org/rGade3f1ccd807

Ah it's just a typoed format (and lack of curly braces around one of the format specifiers)

Fixed in https://reviews.llvm.org/rGade3f1ccd807

Thanks, for a fix. Sorry missed it.