This is an archive of the discontinued LLVM Phabricator instance.

Finish breaking the dependency from lldbUtility -> Host
ClosedPublic

Authored by zturner on Feb 14 2017, 2:23 PM.

Details

Summary

This patch finally gets lldbUtility to be dependency-free. This was done through the following changes:

  1. PseudoTerminal.cpp : Utility -> Host
  2. Introduce a vasprintf / vsnprintf wrapper into utility, and standardize all callers inside of Utility to use this.

With the introduction of #2, we should eventually purge all uses of vsnprintf and vasprintf from the code and instead rely on VASprintf inside of Utility. It is less error prone and more efficient than doing it manually. Unit tests are provided to ensure its correctness, and in writing these unit tests 2 bugs were uncovered in the original implementation, both fixed. (Long term, of course, I would prefer moving to formatv, but in any case, using lldb_private::VASprintf for now is better than using vasprintf / vsnprintf.

After this patch, UtilityTests no longer links against any library other than lldbUtility, as evidenced by the following:

D:\src\llvmbuild\ninja-mono>ninja -nv UtilityTests
[1/1] cmd.exe /C "cd . && "C:\Program Files (x86)\CMake\bin\cmake.exe" -E vs_link_exe --intdir=tools\lldb\unittests\Utility\CMakeFiles\UtilityTests.dir --manifests  -- C:\PROGRA~2\MICROS~1.0\VC\bin\AMD64_~2\link.exe /nologo tools\lldb\unittests\Utility\CMakeFiles\UtilityTests.dir\ConstStringTest.cpp.obj tools\lldb\unittests\Utility\CMakeFiles\UtilityTests.dir\StringExtractorTest.cpp.obj tools\lldb\unittests\Utility\CMakeFiles\UtilityTests.dir\TaskPoolTest.cpp.obj tools\lldb\unittests\Utility\CMakeFiles\UtilityTests.dir\TimeoutTest.cpp.obj tools\lldb\unittests\Utility\CMakeFiles\UtilityTests.dir\UriParserTest.cpp.obj tools\lldb\unittests\Utility\CMakeFiles\UtilityTests.dir\VASprintfTest.cpp.obj tools\lldb\unittests\Utility\CMakeFiles\UtilityTests.dir\D_\src\llvm-mono\llvm\resources\windows_version_resource.rc.res  /out:tools\lldb\unittests\Utility\UtilityTests.exe /implib:tools\lldb\unittests\Utility\UtilityTests.lib /pdb:tools\lldb\unittests\Utility\UtilityTests.pdb /version:0.0  /machine:X86 /STACK:10000000 /debug /INCREMENTAL /subsystem:console  lib\LLVMSupport.lib lib\LLVMSupport.lib lib\gtest_main.lib lib\gtest.lib lib\lldbUtility.lib ws2_32.lib rpcrt4.lib lib\LLVMSupport.lib psapi.lib shell32.lib ole32.lib uuid.lib lib\LLVMDemangle.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib && cmd.exe /C "cd /D D:\src\llvmbuild\ninja-mono\tools\lldb\unittests\Utility && "C:\Program Files (x86)\CMake\bin\cmake.exe" -E make_directory D:/src/llvmbuild/ninja-mono/tools/lldb/unittests/Utility/Inputs""

and I confirmed that introducing a dependency from Utility -> Host causes a linker failure.

lldbUtility.lib(VASprintf.cpp.obj) : error LNK2019: unresolved external symbol "public: static void __cdecl lldb_private::HostInfoWindows::Initialize(void)" (?Initialize@HostInfoWindows@lldb_private@@SAXXZ) referenced in function "void __cdecl lldb_private::VASprintf(class llvm::SmallVectorImpl<char> &,char const *,char *)" (?VASprintf@lldb_private@@YAXAAV?$SmallVectorImpl@D@llvm@@PBDPAD@Z)

Diff Detail

Repository
rL LLVM

Event Timeline

zturner created this revision.Feb 14 2017, 2:23 PM
labath added inline comments.Feb 15 2017, 5:54 AM
lldb/source/Utility/VASprintf.cpp
18 ↗(On Diff #88434)

It doesn't look like you should need to allocate a stack object with the string every time. Can't you just assign buf = "..." in the error case?

28 ↗(On Diff #88434)

Could you also have the function return false in case of an error? You can leave the callers ignoring it, as they weren't handling these errors anyway, but now at least they would have a chance. I know this is meant to go away in the future, but noone know how soon will that be, and it seems like a bad idea to introduce a new abstraction with such an obviously flawed interface (as in, the only way to check if it failed is to inspect the printed-to string).

zturner updated this revision to Diff 88626.Feb 15 2017, 4:30 PM
labath accepted this revision.Feb 16 2017, 7:28 AM

Thanks.

Really splitting hairs now, but could you also update the test to check for the return value. :)

This revision is now accepted and ready to land.Feb 16 2017, 7:28 AM
clayborg requested changes to this revision.Feb 16 2017, 8:53 AM

Please add Xcode changes I sent you offline and this will be good to go.

This revision now requires changes to proceed.Feb 16 2017, 8:53 AM
This revision was automatically updated to reflect the committed changes.
lldb/trunk/source/Utility/Error.cpp