In preparation for eanbling 64bit support in LLDB switching to use llvm::formatv
instead of format MACROs.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 | ||
---|---|---|
827–848 | Let's not keep both. | |
850 | 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 | ||
1170–1175 | This is not using msg. Missing strm.PutCString(msg);? | |
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp | ||
14–55 | Unrelated change? |
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 | ||
---|---|---|
514 | 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.
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.
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp | ||
---|---|---|
14–55 | 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? |
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.
This is looking much better -- and focused. I still have comments though. :)
lldb/include/lldb/Core/Module.h | ||
---|---|---|
833–838 | 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 | |
842 | 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 | ||
1158–1165 | duplicated line | |
lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp | ||
21–28 | ||
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp | ||
198 | this shouldn't be necessary | |
lldb/source/Plugins/SymbolFile/DWARF/DWARFTypeUnit.cpp | ||
18–24 | same here | |
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp | ||
586–587 | 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 | ||
14–55 | 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). | |
3061 | delete empty line | |
lldb/source/Symbol/DWARFCallFrameInfo.cpp | ||
776–782 |
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 |
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.
I'm sorry for the delay, I was out for the holidays. Looks now, thanks for your patience.
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
Ah it's just a typoed format (and lack of curly braces around one of the format specifiers)
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