[Lldb-commits] [PATCH] D76758: Augment lldb's symbol table with external symbols in Mach-O's dyld trie

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

I only have a bunch of superficial comments but this seems reasonable as far as I can tell.



================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:1988
+    bool add_this_entry = false;
     if (EXPORT_SYMBOL_FLAGS_REEXPORT & e.entry.flags && import_name &&
         import_name[0]) {
----------------
`Flags(e.entry.flags).Test(EXPORT_SYMBOL_FLAGS_REEXPORT)` ?


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:1990
         import_name[0]) {
+      // add symbols that are reexport symbols with a valid import name
+      add_this_entry = true;
----------------
Super nit-picky nit pick:

`// Add symbols that are reexport symbols with a valid import name.`


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:2012
       }
-      output.push_back(e);
+      if (EXPORT_SYMBOL_FLAGS_REEXPORT & e.entry.flags) {
+        reexports.push_back(e);
----------------
`Flags(e.entry.flags).Test(EXPORT_SYMBOL_FLAGS_REEXPORT)` ?


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4496
+                              const TrieEntryWithOffset &b) -> bool {
+      return b.entry.address < a.entry.address;
+    };
----------------
Ah I see. the normal `operator<` sorts by offset.


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4499
+    std::sort(external_sym_trie_entries.begin(),
+              external_sym_trie_entries.end(), sort_by_address);
+    for (uint32_t i = 0; i < sym_idx; i++) {
----------------
Do we need llvm::stable_sort here to have reproducible behavior or does it not matter?


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4500
+              external_sym_trie_entries.end(), sort_by_address);
+    for (uint32_t i = 0; i < sym_idx; i++) {
+      TrieEntryWithOffset ent(0);
----------------
Does `for (auto &current_sym : sym)` work here?


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4512
+      if (is_arm && sym[i].GetType() == eSymbolTypeCode &&
+          (sym[i].GetFlags() & MACHO_NLIST_ARM_SYMBOL_IS_THUMB)) {
+        ent.entry.address = symbol_lookup_file_addr + 1;
----------------
`GetFlags().test(MACHO_NLIST_ARM_SYMBOL_IS_THUMB)` ?

Up to taste; it's more verbose but it's kind of confusing to combine && and & in the same expression.


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4514
+        ent.entry.address = symbol_lookup_file_addr + 1;
+        const auto it = std::lower_bound(external_sym_trie_entries.begin(),
+                                         external_sym_trie_entries.end(), ent,
----------------
This block looks like it could be a lambda because it's the same code as above?


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4546
+    bool symbol_is_thumb = false;
+    if (is_arm && e.entry.address & 1) {
+      symbol_is_thumb = true;
----------------
Shows that I don't know the operater precedence rules of C very well, but without extra parenthesis this makes me uneasy :-)


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4554
+      FunctionStarts::Entry *func_start_entry =
+          function_starts.FindEntry(e.entry.address, !is_arm);
+      if (func_start_entry) {
----------------
Wouldn't this automatically fail if `function_starts_count == 0`?


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4556
+      if (func_start_entry) {
+        if (symbol_is_thumb == false &&
+            func_start_entry->addr == e.entry.address) {
----------------
`if (!symbol_is_thumb` ?


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4566
+
+    Address symbol_addr;
+    if (module_sp->ResolveFileAddress(e.entry.address, symbol_addr)) {
----------------
High-level comment what this block is doing?


================
Comment at: lldb/test/API/macosx/dyld-trie-symbols/Makefile:10
+a.out: main.o
+	$(CC) $(CFLAGS_NO_DEBUG) -dynamiclib -o a.out main.o
+
----------------
What does `$(CFLAGS_NO_DEBUG)` achieve in the *linker* invocation?

Could you get rid of this entire rule and just specify `LD_EXTRAS = -dynamiclib`?


================
Comment at: lldb/test/API/macosx/dyld-trie-symbols/Makefile:17
+main.o: main.cpp
+	$(CC) $(CFLAGS_NO_DEBUG) -c $< -o $@
----------------
Same question here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76758