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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
