[PATCH] D53379: GSYM symbolication format
Jonas Devlieghere via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 5 14:58:56 PST 2018
JDevlieghere added a comment.
Thanks Greg! I came across a few more places that don't need braces but otherwise this looks good style-wise.
================
Comment at: include/llvm/DebugInfo/GSYM/DwarfTransformer.h:37
+ if (auto Binary = getObjectFile(filename)) {
+ return loadDwarf(*Binary.getValue().getBinary());
+ }
----------------
No braces
================
Comment at: include/llvm/DebugInfo/GSYM/DwarfTransformer.h:45
+ if (auto Binary = getObjectFile(filename)) {
+ return loadSymbolTable(*Binary.getValue().getBinary());
+ }
----------------
No braces
================
Comment at: include/llvm/DebugInfo/GSYM/InlineInfo.h:37
+
+ // Decode all InlineInfo from address info data.
+ // Returns true if successful, false if InlineInfo is empty.
----------------
`///` for Doxygen comments?
================
Comment at: include/llvm/DebugInfo/GSYM/LineTable.h:27
+enum LineTableOpCode {
+ LTOC_EndSequence = 0x00, // End of the line table
+ LTOC_SetFile = 0x01, // Set LineTableRow.file_idx, don't push a row
----------------
You should considering using `///<` so it becomes a Doxygen comment.
================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:73
+ const ArrayRef<uint8_t> MachUUID = MachO->getUuid();
+ if (!MachUUID.empty()) {
+ UUID.assign(MachUUID.data(), MachUUID.data() + MachUUID.size());
----------------
No braces
================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:81
+ Sect.getName(SectName);
+ if (SectName == GNUBuildID) {
+ StringRef BuildIDData;
----------------
You could make this an early break by inverting the condition. Same for the condition below. I think that would improve the readability of this code.
================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:109
+ if (DWARFDie SpecParent = GetParentDeclContextDIE(SpecDie)) {
+ return SpecParent;
+ }
----------------
No braces
================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:114
+ Die.getAttributeValueAsReferencedDie(dwarf::DW_AT_abstract_origin)) {
+ if (DWARFDie AbstParent = GetParentDeclContextDIE(AbstDie)) {
+ return AbstParent;
----------------
No braces
================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:127
+ DWARFDie ParentDie = Die.getParent();
+ if (!ParentDie) {
+ return DWARFDie();
----------------
No braces
================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:152
+ nullptr)) {
+ return LinkageName;
+ }
----------------
No braces
================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:167
+ // in some binaries. This should hurt, so let's do it for C as well
+ Language == dwarf::DW_LANG_C)) {
+ return ShortName.str();
----------------
No braces
================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:174
+ if (ShortName.startswith("_Z") &&
+ (ShortName.contains(".isra.") || ShortName.contains(".part."))) {
+ return ShortName.str();
----------------
No braces
================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:390
+ }
+ for (DWARFDie ChildDie : Die.children()) {
+ handleDie(OS, CUI, ChildDie);
----------------
No braces
================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:406
+ for (const object::SectionRef &Sect : Obj.sections()) {
+ if (Sect.isText()) {
+ const uint64_t Size = Sect.getSize();
----------------
Simplify this with early break.
================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:421
+
+ if (!Initialized) {
+ initDataFromObj(Obj);
----------------
No braces
================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:443
+ // its local data.
+ for (const auto &CU : DICtx->compile_units()) {
+ CU->getAbbreviations();
----------------
No braces
================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:448
+ for (const auto &CU : DICtx->compile_units()) {
+ pool.async([&CU]() { CU->getUnitDIE(false /*CUDieOnly*/); });
+ }
----------------
No braces
================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:482
+ using namespace llvm::object;
+ if (!Initialized) {
+ initDataFromObj(Obj);
----------------
No braces
================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:496
+ if (Expected<StringRef> Name = Sym.getName()) {
+ Gsym.addFunctionInfo(FunctionInfo(addr, size, Gsym.insertString(*Name)));
+ }
----------------
No braces
================
Comment at: lib/DebugInfo/GSYM/FileTableCreator.cpp:34
+ if (R.second) { // if newly inserted
+ FileEntries.emplace_back(Entry);
+ }
----------------
No braces
================
Comment at: lib/DebugInfo/GSYM/FunctionInfo.cpp:25
+ OS << "Lines:\n";
+ for (const auto &Line : Lines) {
+ Line.dump(OS);
----------------
No braces
================
Comment at: lib/DebugInfo/GSYM/GsymReader.cpp:60
+
+std::string Header::getError() const {
+ // TODO: support swapped GSYM files
----------------
I strongly prefer wrapping this in an llvm::StringError. It's a little more verbose but keeps things consistent.
================
Comment at: lib/DebugInfo/GSYM/GsymReader.cpp:255
+ for (uint32_t I = 0; I < GSYMHeader->NumAddresses; ++I) {
+ dumpAddressInfo(OS, I);
+ }
----------------
No braces
================
Comment at: lib/DebugInfo/GSYM/InlineInfo.cpp:96
+ if (StartAddr <= *LookupAddr && *LookupAddr < EndAddr) {
+ Inline.Ranges.emplace_back(AddressRange(StartAddr, EndAddr));
+ }
----------------
No braces
https://reviews.llvm.org/D53379
More information about the llvm-commits
mailing list