Robinson, Paul via llvm-dev
2017-Apr-12 21:50 UTC
[llvm-dev] [PATCH] D30785: [DWARF] Versioning for DWARF constants; verify FORMs
Honestly, once you know which attribute/form combination has a problem, it's really not too hard to track down where it gets emitted. Grep is your friend. Once I realized this version would require a virtual method I was not happy, but carried through to see where it would lead. Going back to the original location (during abbrev emission) is cheap and easy, and with the debugging output it will be sufficient. --paulr From: llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] On Behalf Of David Blaikie via llvm-commits Sent: Wednesday, April 12, 2017 11:40 AM To: reviews+D30785+public+750a0f0570653e04 at reviews.llvm.org; aprantl at apple.com; clayborg at gmail.com Cc: llvm-commits at lists.llvm.org Subject: Re: [PATCH] D30785: [DWARF] Versioning for DWARF constants; verify FORMs Adding a vtable to DIE's probably not OK - memory usage there is, I think, a bit of a concern. Adrian - thoughts on where this check might be best placed? It'd seem best to make sure it fails at the time the attribute is added to the DIE, rather than during a later walk - but sure, no big deal, probably doesn't come up all that often/not /too/ painful to debug (look at the attribute type, the DIE memory address, set a conditional breakpoint on DIEValueList::addValue & run again). There's a relatively small handful of functions that benefit from this generality, and it might be easy enough to split them (might not be, too) - given that the generality is over blocks and locations that don't have attribute kinds - they're not very much the same thing.... dunno though. On Tue, Apr 11, 2017 at 4:22 PM Paul Robinson via Phabricator <reviews at reviews.llvm.org<mailto:reviews at reviews.llvm.org>> wrote: probinson updated this revision to Diff 94905. probinson added a comment. Move the version check to DIE::addValue(). This required making the base-class method virtual, because the base class doesn't have a way to find the DWARF version; only a DIE can do that. Other review comments addressed as well. https://reviews.llvm.org/D30785 Files: include/llvm/CodeGen/DIE.h include/llvm/ObjectYAML/DWARFYAML.h include/llvm/Support/Dwarf.def include/llvm/Support/Dwarf.h lib/CodeGen/AsmPrinter/DIE.cpp lib/Support/Dwarf.cpp test/DebugInfo/AMDGPU/pointer-address-space-dwarf-v1.ll test/DebugInfo/AMDGPU/variable-locations-dwarf-v1.ll tools/dsymutil/DwarfLinker.cpp unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170412/255b3a42/attachment.html>
Robinson, Paul via llvm-dev
2017-Apr-12 21:57 UTC
[llvm-dev] [PATCH] D30785: [DWARF] Versioning for DWARF constants; verify FORMs
Accidentally cc'd llvm-dev instead of llvm-commits, sorry for the noise! From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of Robinson, Paul via llvm-dev Sent: Wednesday, April 12, 2017 2:50 PM To: David Blaikie; reviews+D30785+public+750a0f0570653e04 at reviews.llvm.org; aprantl at apple.com; clayborg at gmail.com Cc: llvm-dev at lists.llvm.org Subject: Re: [llvm-dev] [PATCH] D30785: [DWARF] Versioning for DWARF constants; verify FORMs Honestly, once you know which attribute/form combination has a problem, it's really not too hard to track down where it gets emitted. Grep is your friend. Once I realized this version would require a virtual method I was not happy, but carried through to see where it would lead. Going back to the original location (during abbrev emission) is cheap and easy, and with the debugging output it will be sufficient. --paulr From: llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] On Behalf Of David Blaikie via llvm-commits Sent: Wednesday, April 12, 2017 11:40 AM To: reviews+D30785+public+750a0f0570653e04 at reviews.llvm.org<mailto:reviews+D30785+public+750a0f0570653e04 at reviews.llvm.org>; aprantl at apple.com<mailto:aprantl at apple.com>; clayborg at gmail.com<mailto:clayborg at gmail.com> Cc: llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org> Subject: Re: [PATCH] D30785: [DWARF] Versioning for DWARF constants; verify FORMs Adding a vtable to DIE's probably not OK - memory usage there is, I think, a bit of a concern. Adrian - thoughts on where this check might be best placed? It'd seem best to make sure it fails at the time the attribute is added to the DIE, rather than during a later walk - but sure, no big deal, probably doesn't come up all that often/not /too/ painful to debug (look at the attribute type, the DIE memory address, set a conditional breakpoint on DIEValueList::addValue & run again). There's a relatively small handful of functions that benefit from this generality, and it might be easy enough to split them (might not be, too) - given that the generality is over blocks and locations that don't have attribute kinds - they're not very much the same thing.... dunno though. On Tue, Apr 11, 2017 at 4:22 PM Paul Robinson via Phabricator <reviews at reviews.llvm.org<mailto:reviews at reviews.llvm.org>> wrote: probinson updated this revision to Diff 94905. probinson added a comment. Move the version check to DIE::addValue(). This required making the base-class method virtual, because the base class doesn't have a way to find the DWARF version; only a DIE can do that. Other review comments addressed as well. https://reviews.llvm.org/D30785 Files: include/llvm/CodeGen/DIE.h include/llvm/ObjectYAML/DWARFYAML.h include/llvm/Support/Dwarf.def include/llvm/Support/Dwarf.h lib/CodeGen/AsmPrinter/DIE.cpp lib/Support/Dwarf.cpp test/DebugInfo/AMDGPU/pointer-address-space-dwarf-v1.ll test/DebugInfo/AMDGPU/variable-locations-dwarf-v1.ll tools/dsymutil/DwarfLinker.cpp unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170412/5a99165d/attachment.html>