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

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.

See inlined comments. You will also need to fix the test for this since it will now fail as the "symbolStatus" now contains the size.



================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:342-349
+    uint64_t debug_info = 0;
+    size_t num_sections = module.GetNumSections();
+    if (num_sections > 0) {
+      for (int i = 0; i < (int)num_sections; i++) {
+        lldb::SBSection section = module.GetSectionAtIndex(i);
+        debug_info += DebugInfoInSection(section);
+      }
----------------
Move these lines to a static function above this function:

```
static uint64_t GetDebugInfoSize(lldb::SBModule module) {
  uint64_t debug_info = 0;
  size_t num_sections = module.GetNumSections();
  for (int i = 0; i < (int)num_sections; i++) {
    lldb::SBSection section = module.GetSectionAtIndex(i);
    debug_info += DebugInfoInSection(section);
  }
  return debug_info;
}
```


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:351
+    if (debug_info > 0) {
+      char debug_info_size[10];
+      if (debug_info < 1024) {
----------------
wallace wrote:
> Move this logic to another function, so that this function is simpler
increase size to 32 just in case we get really big debug info later..


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:353
+      if (debug_info < 1024) {
+        sprintf(debug_info_size, " (%lluKB)", debug_info);
+      } else if (debug_info < 1024*1024) {
----------------
Use the PRIu64 macro here to make sure this works on all platforms and the units are wrong here, should just be "B" instead of "KB". Also use snprintf for safety:
```
snprintf(debug_info_size, sizeof(debug_info_size), " (%"  PRIu64 "B)", debug_info);
```
PRIu64 is a macro that will expand to a string that will always match a uint64_t. The three strings (" (%", PRIu64, and "B)") will get combined by the compiler into a single format string. 


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:355
+      } else if (debug_info < 1024*1024) {
+        debug_info = double(debug_info*10/1024);
+        sprintf(debug_info_size, " (%.1fKB)", double(debug_info/10));
----------------
no need to multiply the debug_info by 10 here. You will want to make a local double so we don't convert back to an integer:

```
double kb = (double)debug_info/1024.0;
```


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:356
+        debug_info = double(debug_info*10/1024);
+        sprintf(debug_info_size, " (%.1fKB)", double(debug_info/10));
+      } else if (debug_info < 1024*1024*1024) {
----------------
wallace wrote:
> debug_info is int, thus, if you do `debug_info/10`, then the result with be an int rounded down. If you cast it to double, you'll have a double without decimals. The correct way to do is to do `double(debug_info) / 10`. 
Just use the local double above "kb" and use snprintf:
```
snprintf(debug_info_size, sizeof(debug_info_size), " (%.1fKB)", kb);
```



================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:358
+      } else if (debug_info < 1024*1024*1024) {
+        debug_info = debug_info*10/(1024*1024);
+        sprintf(debug_info_size, " (%.1fMB)", double(debug_info/10));
----------------
Don't multiply by 10 and use a local double:
```
double mb = (double)debug_info/(1024.0*1024.0);
```


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:359
+        debug_info = debug_info*10/(1024*1024);
+        sprintf(debug_info_size, " (%.1fMB)", double(debug_info/10));
+      } else {
----------------
Use local double and use snprintf:
```
snprintf(debug_info_size, sizeof(debug_info_size), " (%.1fMB)", mb);
```



================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:361
+      } else {
+        debug_info = debug_info*10/(1024*1024*1024);
+        sprintf(debug_info_size, " (%.1fGB)", double(debug_info/10));
----------------
```
double gb = (double)debug_info/(1024.0*1024.0*1024.0);
```



================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:362
+        debug_info = debug_info*10/(1024*1024*1024);
+        sprintf(debug_info_size, " (%.1fGB)", double(debug_info/10));
+      }
----------------
Use local and snprintf:
```
snprintf(debug_info_size, sizeof(debug_info_size), " (%.1fGB)", gb);
```


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:390
 
+uint64_t DebugInfoInSection(lldb::SBSection section) {
+  uint64_t section_size = 0;
----------------
wallace wrote:
> This is a method, therefore it should start with a verb
Rename this as Walter commented and make this function static. You will need to move it above the function above otherwise the compiler won't see it:

```
static uint64_t GetDebugInfoSize(lldb::SBSection section) {
```


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:391
+uint64_t DebugInfoInSection(lldb::SBSection section) {
+  uint64_t section_size = 0;
+  size_t num_sub_sections = section.GetNumSubSections();
----------------
You need to check if the section is a debug info section by checking if it starts with ".debug_" or "__debug":

```
uint64_t debug_info_size = 0;
llvm::StringRef section_name(section.GetName());
if ((section_name.startswith(".debug") || section_name.startswith("__debug"))
  debug_info_size += section.GetFileByteSize();
```


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:393
+  size_t num_sub_sections = section.GetNumSubSections();
+  for (int i = 0; i < (int)num_sub_sections; i++) {
+    lldb::SBSection sub_section = section.GetSubSectionAtIndex(i);
----------------
Use "size_t" as the type for i and remove (int) cast.


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:394-395
+  for (int i = 0; i < (int)num_sub_sections; i++) {
+    lldb::SBSection sub_section = section.GetSubSectionAtIndex(i);
+    section_size += sub_section.GetFileByteSize();
+  }
----------------
This will add the section size of all sections that are contained in a section. We don't want this. We want to recursively call this function and have it compare the section name as mentioned above in the inline comments:
```
  debug_info_size += GetDebugInfoSize(section.GetSubSectionAtIndex(i));
```


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:397
+  }
+  return section_size;
+}
----------------
```
return debug_info_size;
```



================
Comment at: lldb/tools/lldb-vscode/JSONUtils.h:446
 
+uint64_t DebugInfoInSection(lldb::SBSection section);
+
----------------
This isn't a JSON related function. Just make this a static function in the file that uses it and nothing in a header file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83731