This revision was automatically updated to reflect the committed changes.
Closed by commit rL303223: [Expression parser] Look up module symbols before
hunting globally (authored by spyffe).
Changed prior to commit:
https://reviews.llvm.org/D33083?vs=99211=99227#toc
Repository:
rL LLVM
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Very nice.
https://reviews.llvm.org/D33083
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
spyffe updated this revision to Diff 99211.
spyffe added a comment.
Used `CFLAGS_EXTRAS` instead of `CFLAGS` at Greg Clayton's suggestion.
https://reviews.llvm.org/D33083
Files:
include/lldb/Symbol/SymbolContext.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Just a few makefile changes where CFLAGS is being modified and this will be
good to go. Much better location for this. Sean, please keep an eye out for
this kind of thing in the
spyffe updated this revision to Diff 99186.
spyffe added a comment.
Updated to reflect Pavel Labath's suggestions:
- Used `CFLAGS_NO_DEBUG` where appropriate
- Marked the testcase as having no debug info variants
https://reviews.llvm.org/D33083
Files:
include/lldb/Symbol/SymbolContext.h
labath added inline comments.
Comment at: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk:1
+LEVEL := ../../../make
+
Thanks for the effort. It almost works for me :), except for the part where you
clear out the CFLAGS. We cannot do that, as
spyffe updated this revision to Diff 99084.
spyffe added a comment.
Two changes after Greg and Pavel's suggestions:
- Moved the symbol lookup function into the global context; and
- Rewrote the test case's build system to be more generic.
https://reviews.llvm.org/D33083
Files:
labath added a comment.
Is this feature really darwin specific? Isn't the `__private_extern__` thingy
equivalent to `__attribute__(visibility("hidden")))`, which is supported by gcc
and clang alike?
Comment at:
clayborg added a comment.
Yeah, we need to document exactly what this is doing: looking for the best
symbol as an expression parser would want it, preferring symbols from the
current module in the SymbolContext, and then looking everywhere. Errors if
multiple exist in current module, or in any
jingham added a comment.
I think Greg's right. I can't see anything expression parser specific to how
you do this search. I was a little worried that a lot of the other searches
will return multiple names (maybe sorting to indicate closeness.) So this one
might be confusing. But the API
clayborg added a comment.
The searching code is indeed better and what we now want, but I question of
this code should live in ClangExpressionDeclMap::FindGlobalDataSymbol. A better
home for this might be in SymbolContext::FindGlobalDataSymbol? Then others can
use this functionality. Other
spyffe updated this revision to Diff 98672.
spyffe added a comment.
Improved symbol lookup per Greg and Jim's suggestion, with flipped steps 2 and
3.
Also added testing for the error case.
https://reviews.llvm.org/D33083
Files:
spyffe added a comment.
I also would flip 2 and 3. I'll give this a shot.
https://reviews.llvm.org/D33083
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
jingham added a comment.
But note that the real solution to that problem is implementing some syntax for
"symbol from CU", "symbol from Function" etc, as we've discussed in the past.
Then what we do by default will be less important, since you have a way to
override it.
jingham added a comment.
If you just have symbols, you can't tell whether private symbols were visible
to the current frame or not, but the likelihood is not 0 (I guess it's
something like 1/number of CU's). OTOH, your expression could never have seen
a private symbol from another module, and
clayborg added a comment.
Bonus point for implementing a global symbol search that is aware of the above
rules and can stop after step 1 if there are matches, and after step 2 if there
are matches, etc.
https://reviews.llvm.org/D33083
___
clayborg added a comment.
In https://reviews.llvm.org/D33083#751836, @jingham wrote:
> Actually, I take that back. Why do you have to call FindGlobalDataSymbol
> twice? Shouldn't FindGlobalDataSymbol do that work for you. After all you
> passed in the module. It should itself prefer
jingham added a comment.
Actually, I take that back. Why do you have to call FindGlobalDataSymbol
twice? Shouldn't FindGlobalDataSymbol do that work for you. After all you
passed in the module. It should itself prefer symbols in the module...
It also seems wrong that we're just picking the
jingham added a comment.
Yes, this seems obviously right.
https://reviews.llvm.org/D33083
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
clayborg added a comment.
There is a SymbolContext function sorts types based on the context contained in
a SymbolContext. Seems like we should do the same thing here? See
SymbolContext::SortTypeList().
https://reviews.llvm.org/D33083
___
spyffe created this revision.
When it resolved symbol-only variables, the expression parser currently looks
only in the global module list. It should prefer the current module.
I've fixed that behavior by making it search the current module first, and only
search globally if it finds
21 matches
Mail list logo
