[PATCH] D63540: Fix lookup of symbols with the same address range but different binding

Thu Dec 19 03:43:31 PST 2019

labath added a comment.

Defining some sort of a preference based on symbol type seems like a good idea, but I don't think this is a good way to implement it. If I read this patch correctly, then this for example means that the "less global" symbols will not be reported through the Symtab::ForEachSymbolContainingFileAddress API, which seems like a bad thing.

I'm also not happy that this is supposed to be a replacement for the size setting patch, as I believe (and I think we've agreed on that while reviewing the original patch) that *not* fiddling with the sizes of those symbols is a good thing. I don't think reverting a patch that has been sitting in the tree for several months because it "breaks expression evaluation" is a good idea, when that patch has nothing to do with expression evaluation, the failure only happens on a target which has a lot of failures anyway, and the failure only happens on some machine that the original author does not have access to. (I should've said this a while ago, but I was hoping that this problem would be resolved easily..)

Anyway, I think @jankratochvil has done far more than could be expected of any committer to diagnose that problem, and I think it should be up to @omjavaid to explain out how the expression evaluation failures relate to sizeless symbols. At that point, we can re-evaluate whether our decision to *not* fiddle with size of these symbols was correct.

================
Comment at: lldb/test/Shell/SymbolFile/Inputs/symbol-binding.s:10-11
+        .byte   0
+global:
+globalX:
+        .globl  globalX
----------------
I found these names pretty confusing. I'd suggest something like `case1_local`+`case1_global`, `case2_local`+`case2_weak`, etc.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63540