[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB355975: Remove support for DWARF64. (authored by zturner, committed by ). Herald added a project: LLDB. Changed prior to commit: https://reviews.llvm.org/D59235?vs=190173&id=190326#toc Repository:

Re: [Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

Yep yep yep. On Tue, Mar 12, 2019 at 1:24 PM Adrian Prantl via Phabricator wrote: > > aprantl added a comment. > > In D59235#1426716 , @JDevlieghere > wrote: > > > Agreed, and we've been doing this for new patches for a while now. However, > > I very str

[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

aprantl added a comment. In D59235#1426716 , @JDevlieghere wrote: > Agreed, and we've been doing this for new patches for a while now. However, I > very strongly prefer having asserts over "returning a default value", which > only hides real bugs. I t

Re: [Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

+1. I’ve seen some of the changes in llvm that have changed asserts to return a default value. IMHO these should be changed to return Expected instead On Tue, Mar 12, 2019 at 1:16 PM Jonas Devlieghere via Phabricator < revi...@reviews.llvm.org> wrote: > JDevlieghere added a comment. > > In D59235#

[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

JDevlieghere added a comment. In D59235#1426169 , @probinson wrote: > In D59235#1425443 , @zturner wrote: > > > In D59235#1425436 , @clayborg > > wrote: > > > > > My main co

Re: [Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

Fair enough, a FIXME sounds reasonable. On Tue, Mar 12, 2019 at 10:33 AM Jan Kratochvil via Phabricator < revi...@reviews.llvm.org> wrote: > jankratochvil added inline comments. > > > > Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp:263 >uint64_t length

[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

jankratochvil added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp:263 uint64_t length = data.GetU32(&offset); - bool isDwarf64 = (length == 0x); - if (isDwarf64) zturner wrote: > jankratochvil wrote: > > Here

[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

aprantl added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp:89 } llvm_unreachable("invalid UnitType."); } Not your code, but this application of `llvm_unreachable` seems suspicious unless we can guarantee that we a

[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

zturner marked an inline comment as done. zturner added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp:170 case DW_FORM_sec_offset: assert(m_cu); + m_value.value.uval = data.GetMaxU64(offset_ptr, 4); jankrato

[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

zturner marked an inline comment as done. zturner added a comment. It seems like we're mostly in agreement on the direction of the patch, so if there's no outstanding comments I'll submit at the end of the day. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.

[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

jankratochvil added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp:263 uint64_t length = data.GetU32(&offset); - bool isDwarf64 = (length == 0x); - if (isDwarf64) Here should be an error that DWARF64 is not sup

[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

probinson added a comment. In D59235#1425443 , @zturner wrote: > In D59235#1425436 , @clayborg wrote: > > > My main concern with the LLVM DWARF parser is all of the asserts in the > > code. If you attempt to use a

[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

clayborg added a comment. In D59235#1425443 , @zturner wrote: > In D59235#1425436 , @clayborg wrote: > > > My main concern with the LLVM DWARF parser is all of the asserts in the > > code. If you attempt to use a D

[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

zturner added a comment. In D59235#1425436 , @clayborg wrote: > My main concern with the LLVM DWARF parser is all of the asserts in the code. > If you attempt to use a DWARFDIE without first checking it for validity, it > will crash on you instead of ret

[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

clayborg added a comment. My main concern with the LLVM DWARF parser is all of the asserts in the code. If you attempt to use a DWARFDIE without first checking it for validity, it will crash on you instead of returning a good error or default value. That makes me really nervous as we shouldn't

[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

zturner added a subscriber: jankratochvil. zturner added a comment. In D59235#1425417 , @clayborg wrote: > What part of parsing DWARF64 wasn't working? I think the SPARC compiler was > the only one producing DWARF64. This patch originated from a thread

[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

clayborg added a comment. What part of parsing DWARF64 wasn't working? I think the SPARC compiler was the only one producing DWARF64. Be a shame to lose the support. What is the plan here after this patch? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59235/new/ https://reviews.llvm.o

[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

aprantl accepted this revision. aprantl added a comment. This revision is now accepted and ready to land. LGTM, perhaps let's give this a day for others to chime in in case they missed the thread on lldb-dev. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59235/new/ https://reviews.llvm

[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

zturner created this revision. zturner added reviewers: clayborg, jasonmolenda, aprantl. Herald added a subscriber: jdoerfert. LLVM doesn't produce DWARF64, and neither does GCC. LLDB's support for DWARF64 is only partial, and if enabled appears to also not work. It also appears to have no tes