lgtm other than nitpick on formatting. And sorry for delayed review - been sick.

Adding Greg - he is an excellent reviewer and quite responsive (poor guy - ends up with way more than his fair share of reviews as a result :( ).

@spyffe I think this review is waiting on you since you had previously rejected it. Can you please have a look? It continues to show up on my "Blocking Others" list and there's nothing more I can do on my side. Please review or resign out of curtesy to sivachandra??? Thanks in advance.

Amended patch was committed in svn r256877 (svn at lldb.org is back up now)

Amended patch was posted to http://reviews.llvm.org/D15904 - hopefully someone who can get to llvm.org will commit.

I forgot to commit updated changes to the tests:

MiSymbolTestCase.test_lldbmi_symbol_list_lines_file
HelpCommandTestCase.test_help_image_du_line_should_work

so they are failing now, but svn at llvm.org appears to be down at the moment.
I'll commit those changes as soon as svn is back up...

In D15593#319670, @clayborg wrote:

... in the future when adding things for MI, please add an API in the lldb::SB layer to do what you need.

Hopefully folks are back from the holidays by now - can someone give this a look? Thanks in advance...

Replies to Ilia's comments...

In D15593#313342, @clayborg wrote:

Dawn, if you don't understand what I asked you to do,

Updated patch with suggestions from Ilia's review (thanks Ilia!).

clang-format your changes please (there are many deviations from the coding style)

I'm only going to support the combinations:

--compile-unit-only true --only-explicit-matches false --show-raw true
--compile-unit-only false --only-explicit-matches true --show-raw false

OK?

In D15593#312665, @jingham wrote:

So it seems like your change is adding three flags

--compile-unit-only --only-explicit-matches --show-raw

or something. The current behavior is:

--compile-unit-only true --only-explicit-matches false --show-raw true

and your command does:

--compile-unit-only false --only-explicit-matches true --show-raw false

Does that seem right.

In D15593#312571, @jingham wrote:

I don't think this is sufficiently different from "target modules dump line-tables" to warrant a separate command. This seems more like a candidate for a flag to "target modules dump line-tables" like --search-for-inlines or something. That would keep the command surface more manageable and also make the option more discoverable, since you can get both variants in one place.

I first thought about adding it as an option to the line-table but decided against it because the information provided by each are just too different.

Updated patch to include changes to TestHelp.py.

Can a "code owner" please review this patch? Thanks, -Dawn

Updated patch which changes Mangled::GuessLanguage to use the same algorithm for detecting the language from the mangled name as was added to LanguageRuntime::GuessLanguageForSymbolByName.

ping?

Updated Clang-specific version of patch.

This version of the patch is the API "generic" version of the patch. Please choose to review which ever is preferred, and I will submit another patch based on that version if needed.

This version of the patch makes the API specific to clang.

It seems like you combined "find this decl instance within a decl context" with "find a decl by name in the CompilerDeclContext and optionally get the type".

Hi Greg, I'm working on a new revision to change the patch as you suggest (thanks for your review - you had some great suggestions!).

Thanks Greg. Will fix comment in commit.

Please reconsider. Thanks.

(Added Greg - he wrote Mangled::GetLanguage - now GuessLanguage).

In D15326#305163, @jingham wrote:

The part of this fix which is using info in the SymbolContext to make the language detection more accurate is fine.

We have to do something better about how we detect the language from a mangled name. The target knows some that it should be able to contribute, but there's no clear way to do that now. But you aren't making that any worse (except the one inline comment about cstring_is_mangled above, which will need to get fixed) so I don't think you are obligated to solve that problem now.

Greg: But going forward I would like to see more of "find a struct named 'X'

in CompilerDeclContext 'Y'" queries, instead of ...

Thanks Greg! To address your main point:

So either make it generic, or clang specific.

Updated patch to removed change related to Pascal language - it should be part of a separate patch.

In D12809#301752, @sivachandra wrote:

@dawn: Thanks for accepting the patch. I guess I still need to wait for sign-off/comments from a CODE_OWNER.

Jim, this patch doesn't attempt to fix the issue I raised in my comment - it just fixes the oversight of other C variants (clang picks C99 for example) and renames your function to a more meaningful name (and matches Mangled::GuessLanguage which is also uses the name to guess the language). I see no reason why to reject it.

lgtm. Patch was applied locally and tested with no regressions. Thanks for fixing the patch.

Not really an acceptance, but the test that started failing as a result of this commit (LaunchInTerminalTestCase.test_launch_in_terminal) has been XFAILed o OSX in svn.252699 thanks to Todd Fiala. None the less, it would be a good idea to investigate why this commit caused the test to start failing.

In D14631#289917, @tberghammer wrote:

How you end up in calling DWARFFormValue::Address() with a value what have a form type other then DW_FORM_addr and DW_FORM_GNU_addr_index? What is the attribute tag and the form type used? Can you post a call stack for the case when you hit this issue?

If you have a compiler that is actually emitting addresses incorrectly using DW_FORM_data* when the values are not offsets, the compiler should be fixed.

See inline comment.

In D14522#287229, @dblaikie wrote:

Seems OK - if there's any reference documentation you can point to for these attributes please include a reference in the code and/or the commit message.

ping? Please accept - patches to lldb are waiting on this. Thanks.

Patch is correct - without it we loop through the same decl context again. I've tested it locally - there are no new regressions.

1. Please rebase again - this patch no longer applies cleanly, and fails to build after fixing merge conflicts.
2. Please add tests.

Eugene has been doing some work in this area - perhaps he can accept this patch for you.

This was committed in svn r251820.

In D12115#248069, @chaoren wrote:

Could you please take a look at these issues?

TestMiSymbol started to fail on Linux with gcc-4.9:
http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/6387

Breakage with older cmake versions fixed in svn r251073.

Please review http://reviews.llvm.org/D13995 for a proposed fix.

One resolution would be to check for the cmake version and conditionally enable the new code, but that's beyond my capabilities - anyone know how? If not, can we please revert this commit?

First, sorry for my delay in reviewing your patch, but I've been on vacation and now am at the C++ ANSI meeting in Kona for this week. But...

This test is no longer failing.

The filing test was removed by Greg in r249613.

Works great! Please commit. (Sorry for delay - I ran into problems with clang crashing on me for unknown reasons - finally got the build working again so I could test).

Hi Bruce, thanks for this patch! I'll give it a try now...

Build fixed in svn r249684.

FYI - our Jenkins master build just picked up the change and is finally building again - yay!! :)

In D13535#262672, @brucem wrote:

I don't know what's broken for you. My build hasn't had any issues on Mac OS X using cmake.

In D13535#262670, @brucem wrote:

The way to do this correctly is something like
...

I went ahead and committed this since the build was completely broken - I'd be happy to apply any suggestions folks make here in subsequent commits.

What other details are you looking for?

Can someone accept this patch so I can commit please?

Works fine on Linux too.

In D13535#262225, @zturner wrote:

I can't verify that this works on MacOSX at the moment. Did you verify that this works on other platforms as well?

See http://reviews.llvm.org/D13535 for a workaround.

This broke the lldb build on OSX using cmake, when starting from a clean workspace. After this commit, we get:

CMake Error at cmake/modules/AddLLVM.cmake:401 (add_library):
  Cannot find source file:

Test TestTerminal.LaunchInTerminalTestCase.test_launch_in_terminal is still failing. Can you please have a look? Thanks.

This test is still failing. Can you please have a look? Thanks.

This test is still failing. Can you please have a look? Thanks.

This test is still failing. Can you please have a look? Thanks.

This patch is good to commit now right? It's not marked "accepted" for some reason.

You can use clang-format to follow the LLDB coding style, or just do the following:

Can someone please accept and commit this patch so we can use this feature? (After the objections are fixed of course). Thanks.

Was this patch run on the entire set of lldb tests? Were there any regressions?

This patch fixes bugs:

• https://llvm.org/bugs/show_bug.cgi?id=24994 (::val gets NS::val inside NS since r247746)
• https://llvm.org/bugs/show_bug.cgi?id=24995 (shadowed var gets ambiguity since r247836)

Please apply requested changes, then it's good to commit. Thanks for fixing this!

Bug https://llvm.org/bugs/show_bug.cgi?id=24995 opened for "regressions" after this commit.

Clang (the LLDB compiler) does not ask for the value ScNSpacGl in the global namespace, but just for a value named ScNSpacGl.