This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Fix incorrect uses of formatv specifiers in LLDB_LOG
ClosedPublic

Authored by JDevlieghere on Jul 5 2023, 10:51 AM.

Details

Summary

Fix incorrect uses of formatv specifiers in LLDB_LOG. Unlike Python, arguments must be numbered. All these log statements take llvm:Errors so use the LLDB_LOG_ERROR macro instead.

Diff Detail

Event Timeline

JDevlieghere created this revision.Jul 5 2023, 10:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2023, 10:51 AM

These uses of LLDB_LOG are taking errors, consuming them while turning them into strings, and logging them. Unless I'm missing something, I think it would make more sense to convert these to use LLDB_LOG_ERROR.

lldb/source/Core/Debugger.cpp
1918–1919

Same here.

bulbazord added inline comments.Jul 5 2023, 11:17 AM
lldb/source/Core/Debugger.cpp
1918–1919

Sorry, ignore this comment, I deleted a previous inline comment and forgot to delete this one.

These uses of LLDB_LOG are taking errors, consuming them while turning them into strings, and logging them. Unless I'm missing something, I think it would make more sense to convert these to use LLDB_LOG_ERROR.

Good point, I blindly fixed the format specifiers without lookin what was being passed in.

JDevlieghere edited the summary of this revision. (Show Details)

In https://reviews.llvm.org/D154532, the string argument comes after the error. Are these interchangeable?

fdeazeve accepted this revision.Jul 5 2023, 11:25 AM

Ah you updated the review as I typed the question! LGTM

This revision is now accepted and ready to land.Jul 5 2023, 11:25 AM
bulbazord requested changes to this revision.Jul 5 2023, 11:25 AM

The 2nd argument to LLDB_LOG_ERROR is the error itself, so these should be something like LLDB_LOG_ERROR(GetLog(whatever), std::move(err), "message: {0}"); or something to this effect.

This revision now requires changes to proceed.Jul 5 2023, 11:25 AM
bulbazord accepted this revision.Jul 5 2023, 11:25 AM

I see you updated it while I typed it up. LGTM.

This revision is now accepted and ready to land.Jul 5 2023, 11:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2023, 11:28 AM