This is an archive of the discontinued LLVM Phabricator instance.

Allow signposts to take advantage of deferred string substitution
ClosedPublic

Authored by aprantl on Jun 2 2021, 5:29 PM.

Details

Summary

This is more an RFC at this point. I'm curious about feedback!

One nice feature of the os_signpost API is that format string substitutions happen in the consumer, not the logging application. LLVM's current Signpost class doesn't take advantage of this though and instead always uses a static "Begin/End %s" format string.

This patch is proof of concept for how to use variadic macros to allow the API to be used as intended. Unfortunately, the primary use-case I had in mind (the LLDB_SCOPED_TIMER() macro) does not get much better from this, because __PRETTY_FUNCTION__ is *not* a macro, but a static string, so signposts created by LLDB_SCOPED_TIMER() still use a static "%s" format string. At least LLDB_SCOPED_TIMERF() works as intended.

Diff Detail

Event Timeline

aprantl created this revision.Jun 2 2021, 5:29 PM
aprantl requested review of this revision.Jun 2 2021, 5:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2021, 5:29 PM

I think we should just remove the functionality form the timer class again. I only added it there because of the macro.

I'm not sure I fully understand the suggestion:

I think we should just remove the functionality form the timer class again. I only added it there because of the macro.

... and replace its uses with what?

I'm not sure I fully understand the suggestion:

I think we should just remove the functionality form the timer class again. I only added it there because of the macro.

... and replace its uses with what?

I'm suggesting we no longer emit a signpost from the lldb_private::Timer class and instead exclusively emit a signpost from the LLDB_SCOPED_TIMER macro. We barely have any places where someone creates a timer directly and this would help discourage that further. The only reason it's currently in the timer class it because it seemed cleaner than doing it in the macro directly. Since we now have to emit the signpost from the LLDB_SCOPED_TIMER macro anyway, there's no reason to keep the functionality in the Timer class.

aprantl updated this revision to Diff 351332.Jun 10 2021, 7:03 PM

Address review feedback from @JDevlieghere.

Let me know if this goes into the right direction.

JDevlieghere accepted this revision.Jun 11 2021, 10:48 AM

Looks good to me!

This revision is now accepted and ready to land.Jun 11 2021, 10:48 AM
This revision was landed with ongoing or failed builds.Jun 11 2021, 4:35 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2021, 4:35 PM
fhahn added a subscriber: fhahn.Jun 12 2021, 4:10 AM

It looks like this is causing build failures on certain macOS / SDK combinations, e.g. http://green.lab.llvm.org/green/job/lldb-cmake-standalone/3288/consoleFull#-195476041949ba4694-19c4-4d7e-bec5-911270d8a58c

I reverted the change for now in b4583a5ad73b

/Users/buildslave/jenkins/workspace/lldb-cmake-standalone/host-compiler/bin/clang++  -DGTEST_HAS_RTTI=0 -DHAVE_ROUND -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Isource/Plugins/ObjectFile/Mach-O -I/Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/lldb/source/Plugins/ObjectFile/Mach-O -Isource -I/Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/lldb/include -Iinclude -I/Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/llvm/include -I/Users/buildslave/jenkins/workspace/lldb-cmake-standalone/clang-build/include -I/Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/clang/include -I/Users/buildslave/jenkins/workspace/lldb-cmake-standalone/clang-build/tools/clang/include -I/usr/local/Cellar/python/3.7.0/Frameworks/Python.framework/Versions/3.7/include/python3.7m -I/Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/lldb/tools/clang/include -I../clang/include -I/usr/local/include -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk/usr/include/libxml2 -I/Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/lldb/source/. -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -fdiagnostics-color -Wno-deprecated-declarations -Wno-unknown-pragmas -Wno-strict-aliasing -Wno-deprecated-register -Wno-vla-extension -O3 -DNDEBUG -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk    -fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -MT source/Plugins/ObjectFile/Mach-O/CMakeFiles/lldbPluginObjectFileMachO.dir/ObjectFileMachO.cpp.o -MF source/Plugins/ObjectFile/Mach-O/CMakeFiles/lldbPluginObjectFileMachO.dir/ObjectFileMachO.cpp.o.d -o source/Plugins/ObjectFile/Mach-O/CMakeFiles/lldbPluginObjectFileMachO.dir/ObjectFileMachO.cpp.o -c /Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
In file included from /Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:45:
In file included from /Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/lldb/include/lldb/Host/SafeMachO.h:159:

/Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/llvm/include/llvm/BinaryFormat/MachO.h:30:3: error: expected identifier
  MH_MAGIC = 0xFEEDFACEu,
  ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk/usr/include/mach-o/loader.h:65:18: note: expanded from macro 'MH_MAGIC'
#define MH_MAGIC        0xfeedface      /* the mach magic number */
                        ^
In file included from /Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:45:
In file included from /Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/lldb/include/lldb/Host/SafeMachO.h:159:
/Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/llvm/include/llvm/BinaryFormat/MachO.h:31:3: error: expected identifier
  MH_CIGAM = 0xCEFAEDFEu,
  ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk/usr/include/mach-o/loader.h:66:18: note: expanded from macro 'MH_CIGAM'
#define MH_CIGAM        0xcefaedfe      /* NXSwapInt(MH_MAGIC) */
                        ^
In file included from /Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:45:
In file included from /Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/lldb/include/lldb/Host/SafeMachO.h:159:
/Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/llvm/include/llvm/BinaryFormat/MachO.h:32:3: error: expected identifier
  MH_MAGIC_64 = 0xFEEDFACFu,
  ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk/usr/include/mach-o/loader.h:84:21: note: expanded from macro 'MH_MAGIC_64'
#define MH_MAGIC_64 0xfeedfacf /* the 64-bit mach magic number */
                    ^
In file included from /Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:45:
In file included from /Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/lldb/include/lldb/Host/SafeMachO.h:159:
/Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/llvm/include/llvm/BinaryFormat/MachO.h:33:3: error: expected identifier
  MH_CIGAM_64 = 0xCFFAEDFEu,
  ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk/usr/include/mach-o/loader.h:85:21: note: expanded from macro 'MH_CIGAM_64'
#define MH_CIGAM_64 0xcffaedfe /* NXSwapInt(MH_MAGIC_64) */
                    ^
In file included from /Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:45:
In file included from /Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/lldb/include/lldb/Host/SafeMachO.h:159:
/Users/buildslave/jenkins/workspace/lldb-cmake-standalone/llvm-project/llvm/include/llvm/BinaryFormat/MachO.h:43:3: error: expected identifier
  MH_OBJECT = 0x1u,
  ^

The Windows buildbot does not like signposts:

https://lab.llvm.org/buildbot/#/builders/83/builds/7271

C:\PROGRA~2\MICROS~3\2017\COMMUN~1\VC\Tools\MSVC\1416~1.270\bin\Hostx64\x64\cl.exe  /nologo /TP -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_ENABLE_EXTENDED_ALIGNED_STORAGE -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools\lldb\source\Utility -IC:\buildbot\lldb-x64-windows-ninja\llvm-project\lldb\source\Utility -Itools\lldb\source -IC:\buildbot\lldb-x64-windows-ninja\llvm-project\lldb\include -Itools\lldb\include -Iinclude -IC:\buildbot\lldb-x64-windows-ninja\llvm-project\llvm\include -I"C:\Program Files (x86)\Microsoft Visual Studio\Shared\Python36_64\include" -IC:\buildbot\lldb-x64-windows-ninja\llvm-project\llvm\..\clang\include -Itools\lldb\..\clang\include -IC:\buildbot\lldb-x64-windows-ninja\llvm-project\lldb\source\. /DWIN32 /D_WINDOWS   /Zc:inline /Zc:__cplusplus /Zc:strictStrings /Oi /Zc:rvalueCast /bigobj /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /Gw /MD /O2 /Ob2 /DNDEBUG   -wd4018 -wd4068 -wd4150 -wd4201 -wd4251 -wd4521 -wd4530  /EHs-c- /GR- -std:c++14 /showIncludes /Fotools\lldb\source\Utility\CMakeFiles\lldbUtility.dir\Timer.cpp.obj /Fdtools\lldb\source\Utility\CMakeFiles\lldbUtility.dir\lldbUtility.pdb /FS -c C:\buildbot\lldb-x64-windows-ninja\llvm-project\lldb\source\Utility\Timer.cpp
C:\buildbot\lldb-x64-windows-ninja\llvm-project\lldb\include\lldb/Utility/Timer.h(49): error C3646: '__attribute__': unknown override specifier
C:\buildbot\lldb-x64-windows-ninja\llvm-project\lldb\include\lldb/Utility/Timer.h(49): error C2059: syntax error: '('
C:\buildbot\lldb-x64-windows-ninja\llvm-project\lldb\include\lldb/Utility/Timer.h(50): error C2059: syntax error: '('
C:\buildbot\lldb-x64-windows-ninja\llvm-project\lldb\include\lldb/Utility/Timer.h(53): error C2143: syntax error: missing ')' before ';'
C:\buildbot\lldb-x64-windows-ninja\llvm-project\lldb\include\lldb/Utility/Timer.h(53): error C2059: syntax error: ')'

Looks like it's the attribute that it doesn't know about:

/// Default constructor.
Timer(Category &category, const char *format, ...)
    __attribute__((format(printf, 3, 4)));

which is weird because I didn't actually change that.

I tried to work around it.