[Lldb-commits] [PATCH] D81334: 2/2: [lldb] Fix DW_TAG_GNU_call_site-DW_AT_low_pc as produced by GCC

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFBaseDIE.h:113
 
-  size_t GetAttributes(DWARFAttributes &attributes, uint32_t depth = 0) const;
+  size_t GetAttributes(DWARFAttributes &attributes, int32_t depth = 0) const;
 
----------------
jankratochvil wrote:
> labath wrote:
> > Using -1 to prevent recursion is pretty unobvious. I think it would be better to add a `bool recurse = true` argument between `attributes` and `depth`. In fact, I'd consider even deleting the `depth` argument -- it's an internal detail that noone except `DWARFDebugInfoEntry` should be using and the single usage there can be easily changed to `spec_die.GetDIE()->GetAttributes(spec_die.GetUnit(), attributes, recurse, depth+1)`
> I had this idea. :-) The problem is `bool recurse` is type-compatible with `uint32_t depth` which leads to uncaught bugs. Given that non-recursive call makes no sense with `depth>0` I find the special value `depth = -1` to be suitable for non-recursive calls.
> Anyway implemented the `recurse` parameter in a safe way if it is worth it to separate it.
> 
Right, I see. I don't think that a special tagged enum is really necessary here and flushing out the callers via two patches (like you did) would be enough. The only ones who that may impact are downstream users who merge in bulk, but: a) I doubt the downstream users are calling this function in their own code; b) we don't have to work extra hard to safeguard them.

However, given that you've already done it, I think it's fine to keep it -- though I'll probably change it back next time I work on this function.


================
Comment at: lldb/test/Shell/SymbolFile/DWARF/DW_TAG_GNU_call_site-DW_AT_low_pc.s:46
+	call	b
+        int3
+	ret
----------------
This is formatted strangely. Tab issues?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81334/new/

https://reviews.llvm.org/D81334