teemperor added a comment. I think this is really nice. I have some minor remarks here and there but otherwise this LGTM.
================ Comment at: lldb/source/Plugins/DynamicLoader/WASM-DYLD/DynamicLoaderWasmDYLD.cpp:88 + + if ((module_sp = modules.FindFirstModule(module_spec))) { + UpdateLoadedSections(module_sp, link_map_addr, base_addr, ---------------- what about making this if and the one blow a `if (ModuleSP module_sp = ...) { ...; return module_sp; }`. Then you don't need to do the double-parentheses trick and the end of this function can just be `return ModuleSP();` so it is obvious that the end of this function is the error code path. ================ Comment at: lldb/source/Plugins/DynamicLoader/WASM-DYLD/DynamicLoaderWasmDYLD.cpp:171 + auto arch = m_process->GetTarget().GetArchitecture(); + if (arch.GetMachine() != llvm::Triple::wasm32) { + return ThreadPlanSP(); ---------------- no brackets ================ Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:32 + const char *magic = reinterpret_cast<const char *>(data_sp->GetBytes()); + if (strncmp(magic, llvm::wasm::WasmMagic, sizeof(llvm::wasm::WasmMagic)) != + 0) { ---------------- aprantl wrote: > Is a StringRef comparison easier to read here? (IMHO it is) ================ Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:65 + data_sp = MapFileData(*file, length, file_offset); + if (!data_sp) { + return nullptr; ---------------- no brackets ================ Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:79 + data_sp = MapFileData(*file, length, file_offset); + if (!data_sp) { + return nullptr; ---------------- No brackets ================ Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:88 + ArchSpec spec = objfile_up->GetArchitecture(); + if (spec && objfile_up->SetModulesArchitecture(spec)) { + return objfile_up.release(); ---------------- no brackets ================ Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:110 + +// static +bool ObjectFileWasm::GetVaruint7(DataExtractor §ion_header_data, ---------------- I don't think we do these `static` comments usually in LLVM? ================ Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:128 + uint64_t value = section_header_data.GetULEB128(offset_ptr); + if (*offset_ptr == initial_offset || value > uint64_t(1) << 32) { + return false; ---------------- Single line if -> no brackets needed. ================ Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:172 + } + std::string sect_name(reinterpret_cast<const char *>(name_bytes), + name_len); ---------------- Should also be switched to ConstString if you make the member of the section info a ConstString. ================ Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:191 + } + m_symbols_url = + ConstString(reinterpret_cast<const char *>(url_bytes), url_len); ---------------- This member is only created here and only used below from what I can see? Also you never compare it against any other strings so it should be a `std::string`. ================ Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:292 + for (SectionInfoCollConstIter it = m_sect_infos.begin(); + it != m_sect_infos.end(); ++it) { + const section_info §_info = *it; ---------------- range-based loop ================ Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:299 + section_type = eSectionTypeCode; + else if (g_sect_name_dwarf_debug_abbrev == sect_info.name.c_str()) + section_type = eSectionTypeDWARFDebugAbbrev; ---------------- Your `sect_info.name` is already a std::string so comparing here against a ConstString is just a slower and less readable. ================ Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:447 + ModuleSP module_sp(GetModule()); + if (module_sp) { + std::lock_guard<std::recursive_mutex> guard(module_sp->GetMutex()); ---------------- early-exit ================ Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:449 + std::lock_guard<std::recursive_mutex> guard(module_sp->GetMutex()); + s->Printf("%p: ", static_cast<void *>(this)); + s->Indent(); ---------------- Bonus: You can write this code by directly using `llvm::raw_ostream` by just calling `s->AsRawOstream()` to get the equivalent `raw_ostream`. I migrate all code to LLVM's stream classes so not having more code using lldb::Stream would be nice (but not required to get this patch in). (same for the Stream code below) ================ Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.h:116 + uint32_t id; + std::string name; + } section_info_t; ---------------- This should be a `ConstString`. OR you keep this as a `std::string` and then you move all other ConstString variables that compare against your section names to be just plain `std::string`, `llvm::StringRef` and `llvm::StringSwitch`. But mixing `ConstString` and normal strings brings us the disadvantages of both worlds (hard to read and slow to compare) without any benefits. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71575/new/ https://reviews.llvm.org/D71575 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits