Page MenuHomePhabricator

Expression evaluation for overloaded C functions
AbandonedPublic

Authored by EwanCrawford on Mar 8 2016, 4:03 AM.

Details

Reviewers
spyffe
clayborg
Summary

Fixes bugzilla ticket https://llvm.org/bugs/show_bug.cgi?id=26694
Where we can currently pick the incorrect declaration when calling a C function with the overloadable attribute.

This is done by checking if we're using language C, but the function has a mangled name.
Patch also includes some additional fallback parameter manglings for when clang doesn't emit a matching symbol.

Diff Detail

Repository
rL LLVM

Event Timeline

EwanCrawford retitled this revision from to Expression evaluation for overloaded C functions.
EwanCrawford updated this object.
EwanCrawford added reviewers: spyffe, clayborg.
EwanCrawford set the repository for this revision to rL LLVM.
EwanCrawford added a subscriber: lldb-commits.
clayborg resigned from this revision.Mar 11 2016, 9:19 AM
clayborg removed a reviewer: clayborg.

I will let Sean Callanan review this one as the expression parser is his area.

spyffe requested changes to this revision.Mar 11 2016, 10:29 AM
spyffe edited edge metadata.

There are a few changes I'd recommend to this patch. The biggest one is to move mangling knowledge from IRExecutionUnit to LanguageCPlusPlus, where it logically fits better.
The testsuite change should be applied across the board in my opinion, but I'm going to add Greg Clayton as a reviewer to cover that part.

packages/Python/lldbsuite/test/expression_command/call-overloaded-c-fuction/Makefile
10

Should we do this for all tests? Greg?

source/Expression/IRExecutionUnit.cpp
736

I'm pretty sure this should be on LanguageCPlusPlus, along with the code in CollectCandidateCPlusPlusNames that tries _ZN -> _ZNK and _Z -> _ZL.

LanguageCPlusPlus should have a method that takes a name and collects "equivalent" mangled names. That way all this mangling knowledge is kept in one place.

765

This is going to add SearchSpecs regardless of whether SubstituteMangledParameters did anything, slowing down symbol lookups for cases where it isn't necessary. This should only add specs if there was actually a difference.

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
2099

Two things:

  • Why does this have to happen down here? Couldn't it be done up at the declaration of extern_c so that the bool stays const?
  • Looking at this logic, it might also be cool to also set extern_c if the compile unit is C++ but the function in the DWARF has a non-mangled name... but that's solving a separate problem
This revision now requires changes to proceed.Mar 11 2016, 10:29 AM

Added Greg to look at the testsuite changes.

EwanCrawford edited edge metadata.

Thanks for taking a look Sean.

I moved the mangling logic into LanguageCPlusPlus and created a new method for it.
There was an existing function FindEquivalentNames that used CPPRuntimeEquivalents, but looked like that was targeted at types. Let me know if you prefer I reuse that.

clayborg accepted this revision.Mar 14 2016, 9:51 AM
clayborg edited edge metadata.
clayborg added inline comments.
packages/Python/lldbsuite/test/expression_command/call-overloaded-c-fuction/Makefile
9

Maybe we can have a standard clean rule in the main makefiles:

standard_clean:

rm -rf $(wildcard *.o *.d *.dSYM)

And maybe we can call this before calling the "clean" where each test case can override this. Not required for this patch though.

labath added a subscriber: labath.Mar 14 2016, 9:59 AM
labath added inline comments.
packages/Python/lldbsuite/test/expression_command/call-overloaded-c-fuction/Makefile
9

The main Makefile.rules does the cleanup already, I'm pretty sure this rule is not necessary.

EwanCrawford added inline comments.Mar 14 2016, 10:10 AM
packages/Python/lldbsuite/test/expression_command/call-overloaded-c-fuction/Makefile
9

Good to know, I'll remove this case

EwanCrawford edited edge metadata.

Rebased on tip

EwanCrawford abandoned this revision.Nov 29 2016, 10:13 AM
EwanCrawford added a subscriber: ldrumm.

Abandoning this patch since it's out of date, and @ldrumm has an alternative fix he's about to post

New candidate is here