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 ¤t_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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
