[Lldb-commits] [PATCH] D83731: Add Debug Info Size to Symbol Status

[walter erquinigo via Phabricator via lldb-commits]

wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.

The logic looks very good, just some final comments and the code will be high quality



================
Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:66
+    @skipIfRemote    
+    def test_module_event(self):
+        # Mac or linux.
----------------
With you current Makefile, this test will fail on Linux, as the Makefile will expect a .dsym file to be created. Simply put @ skipUnlessDarwin back here, and add this comment
  #TODO: Update the Makefile so that this test runs on Linux

Once this is committed, I'll work on make this test pass on Linux


================
Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:67-73
+        # Mac or linux.
+
+        # On mac, if we load a.out as our symbol file, we will use DWARF with .o files and we will
+        # have debug symbols, but we won't see any debug info size because all of the DWARF
+        # sections are in .o files.
+
+        # On other platforms, we expect a.out to have debug info, so we will expect a size.
----------------
The common way to write function comments is with ''', like this
  '''
    Mac or linux.

    On mac, if we load a.out as our symbol file, we will use DWARF with .o files and we will
    have debug symbols, but we won't see any debug info size because all of the DWARF
    sections are in .o files.

    On other platforms, we expect a.out to have debug info, so we will expect a size.
  '''

That way you don't need to type that many #


================
Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:81-84
+        # Darwin only test with dSYM file.
+
+        # On mac, if we load a.out.dSYM as our symbol file, we will have debug symbols and we
+        # will have DWARF sections added to the module, so we will expect a size.
----------------
same here about the comment


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:333
+  llvm::StringRef section_name(section.GetName());
+  if (section_name.startswith(".debug") || section_name.startswith("__debug")
+      || section_name.startswith(".apple") || section_name.startswith("__apple"))
----------------
apply the format change it suggests, i.e start the second line with ||


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:353
+
+static std::string ConvertDebugInfoSize(uint64_t debug_info) {
+  char debug_info_size[32];
----------------
ConvertDebugInfoSizeToString is a better name


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:354-368
+  char debug_info_size[32];
+  if (debug_info < 1024) {
+    snprintf(debug_info_size, sizeof(debug_info_size), " (%" PRIu64 "B)",
+             debug_info);
+  } else if (debug_info < 1024 * 1024) {
+    double kb = double(debug_info) / 1024.0;
+    snprintf(debug_info_size, sizeof(debug_info_size), " (%.1fKB)", kb);
----------------
a more modern way to implement this is

  #include <sstream>
  #include <iomanip>
   ...
  std::ostringstream oss;
  oss << "(";
  oss << std::fixed << std::setprecision(1);

  if (debug_info < 1024) {
    oss << debug_info << "B";
  } else if (debug_info < 1024 * 1024) {
    double kb = double(debug_info) / 1024.0;
    oss << kb << "KB";
  } else if (debug_info < 1024 * 1024 * 1024) {
    double mb = double(debug_info) / (1024.0 * 1024.0);
    oss << mb << "MB";
  } else {
    double gb = double(debug_info) / (1024.0 * 1024.0 * 1024.0);
    oss << gb << "GB";;
  }
  oss << ")";
  return oss.str();

It's actually safer, as you don't need to specify the array size of your debug_info_size_buffer


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:385-386
+    if (debug_info > 0) {
+      std::string debug_info_size = ConvertDebugInfoSize(debug_info);
+      symbol_str = symbol_str + debug_info_size;
+    }
----------------
  symbol_str += ConvertDebugInfoSizeToString(debug_info);
is more concise


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83731/new/

https://reviews.llvm.org/D83731