Page MenuHomePhabricator

[LLDB-MI] Fix -data-info-line and -symbol-list-lines when Windows filenames are used.
ClosedPublic

Authored by dawn on Aug 18 2015, 1:52 PM.

Details

Summary

This fixes -data-info-line and -symbol-list-lines to parse the filename and line correctly when line entries don't have the optional column number and the filename contains a Windows drive letter. It also fixes -symbol-list-lines when code from header files is generated.

Diff Detail

Repository
rL LLVM

Event Timeline

dawn updated this revision to Diff 32448.Aug 18 2015, 1:52 PM
dawn retitled this revision from to [LLDB-MI] Fix -data-info-line when Windows filenames are used..
dawn updated this object.
dawn added reviewers: abidh, ki.stfu, brucem.
dawn set the repository for this revision to rL LLVM.
dawn added a subscriber: lldb-commits.

Rather than all of this ugly error-prone code, can we instead use
llvm::sys::fs::root_name to check whether the path contains a drive letter?

dawn added a comment.Aug 18 2015, 3:58 PM

Rather than all of this ugly error-prone code, can we instead use
llvm::sys::fs::root_name to check whether the path contains a drive letter?

Sadly no, because root_name only parses c:\ correctly if the platform is Windows. Our IDE is hosted on Windows but talks to lldb running on OSX. I've had this path problem with the DWARF reader and FileSpec - lldb has a core assumption that you only want to support Windows paths if you run on Windows. At least with FileSpec you can pass a PathSyntax, but you have to choose either Windows or Posix, not both. We want to support both Posix and Windows style paths. I've not come up with a clean patch for this problem so none have been submitted. We should start another thread about how to resolve this problem...

For now, would it be OK to put a FIXME comment in the source?

Are you saying that there is a situation where you are given a path, and
you have no idea whether it is a posix path or a windows path? That seems
strange to me. Surely you must know (or be able to find out) if the
computer you are interfacing with is running Windows or non-Windows, right?

dawn added a comment.Aug 18 2015, 4:49 PM

Are you saying that there is a situation where you are given a path, and you have no idea whether it is a posix path or a windows path?

Yes. For example, in lldb on OSX, we can be debugging an app that was built on OSX, or an app that was built on Windows targeting OSX. More likely, we would be debugging an app that was built on Windows but links with libraries that were built on OSX.

I've thought of various ways to fit this into lldb...
One idea was to add:

settings set target.pathsyntax [posix|windows|any|native] #default=native

and a FileSpec::ePathSyntaxAny enum which would be set if the user chose "any".
But that doesn't solve the problem for llvm, whose path handling appears to be controlled by the define LLVM_ON_WIN32.

Ideas?

brucem edited edge metadata.Aug 18 2015, 4:59 PM

Apart from the concerns expressed by zturner, this needs a test as well if possible.

dawn added a comment.Aug 18 2015, 7:40 PM

... this needs a test as well if possible.

Since the lldb-mi tests don't run on Windows, I don't see how that would be possible.

abidh edited edge metadata.Aug 19 2015, 2:30 AM

This command uses "target modules lookup" to obtain the information while -symbol-list-lines uses "target modules dump line-table". Although there is difference in the format of the output of these 2 commands, the format for path and line is same. I think we should have one code handling path strings in both of these places to avoid duplication.

It would be even better if we have some API which we can use to query this data. Then there would be no need for this parsing.

dawn planned changes to this revision.Aug 19 2015, 12:56 PM

This command uses "target modules lookup" [...]

Good point. Yes, it would be good to encapsulate this. I'll rework this patch accordingly when I can find the time (busy with a release now).

That said, I would love to continue the discussion of how to tackle the filename/path issue so I can have some direction as to how to resolve it. Should that move to lldb-dev?

Seems reasonable to move that to lldb-dev.

dawn updated this revision to Diff 34255.Sep 8 2015, 2:12 PM
dawn retitled this revision from [LLDB-MI] Fix -data-info-line when Windows filenames are used. to [LLDB-MI] Fix -data-info-line and -symbol-list-lines when Windows filenames are used..
dawn updated this object.
dawn edited edge metadata.

This changes the code to use regex to handle the parsing. It is cleaner, more accurate, and faster than the previous code, while not requiring special support to check for Windows paths.

In reworking this patch, I discovered a major bug in -symbol-list-lines where it didn't check the filename so would report lines in header files as belonging to the compilation unit. That is fixed in this patch and a test added, but to fix -symbol-list-lines for header files requires a rewrite of -symbol-list-lines which I didn't have time for. I've added a FIXME for now.

dawn updated this object.Sep 8 2015, 2:15 PM
dawn updated this revision to Diff 34406.Sep 9 2015, 8:32 PM

ping?

I'm probably not the best person for this since I don't know anything about
the MI stuff

abidh requested changes to this revision.Sep 10 2015, 2:17 AM
abidh edited edge metadata.

We dont use the lldb_private stuff inside lldb-mi. Please see the discussion in the following thread.
http://lists.llvm.org/pipermail/lldb-dev/2015-March/007047.html

This revision now requires changes to proceed.Sep 10 2015, 2:17 AM
dawn added a comment.Sep 10 2015, 5:03 PM

We dont use the lldb_private stuff inside lldb-mi. Please see the discussion in the following thread.
http://lists.llvm.org/pipermail/lldb-dev/2015-March/007047.html

Thank you for your review and for the link. I agree in general and have avoided lldb_private myself, but this means duplicated code which is arguably worse. I think it's time to make lldb-mi a proper part of lldb to avoid reinventing the wheel, or adding SB APIs that make no sense outside of lldb-mi. But I am sympathetic to both arguments so will revise.

The FileSpec is not even used for anything is it? If you want to use the
RegularExpression, then use llvm's regular expression. Honestly LLDB's
regular expression should be deprecated anyway, there's no reason to have
an LLDB regex and an LLVM regex when just one will suffice

dawn added a comment.Sep 14 2015, 5:57 PM

The FileSpec is not even used for anything is it?

No, that was a carry over from a previous patch - now deleted.

If you want to use the
RegularExpression, then use llvm's regular expression. Honestly LLDB's
regular expression should be deprecated anyway, there's no reason to have
an LLDB regex and an LLVM regex when just one will suffice

I agree.

But for now, I've written a small regex wrapper for lldb-mi based on llvm's C regex implementation which uses lldb-mi's string types. It would be awesome to replace lldb-mi's types with those of llvm someday (e.g. have CMIUtilString use StringRef instead of std::string, etc), so that it would be easier to use llvm's classes.

dawn updated this revision to Diff 34769.Sep 14 2015, 6:00 PM
dawn edited edge metadata.

Patch fixed to remove uses of lldb_private classes.

abidh requested changes to this revision.Sep 15 2015, 1:53 AM
abidh edited edge metadata.

You forgot to add the MIUtilParse.cpp to CMakeLists.txt. Please add it and then it is good to go. Thanks for doing it.

This revision now requires changes to proceed.Sep 15 2015, 1:53 AM
dawn updated this revision to Diff 34812.EditedSep 15 2015, 10:55 AM
dawn edited edge metadata.

Oops - forgot patch to CMakeLists.txt.

OK to commit now?

ki.stfu requested changes to this revision.Sep 15 2015, 11:32 PM
ki.stfu edited edge metadata.
ki.stfu added inline comments.
test/tools/lldb-mi/symbol/main.cpp
12 ↗(On Diff #34812)

Don't impose restrictions on the main file which contains test cases for symbol-xxx commands. If a new cases should be added, we will be forced to modify this test case (or be sure that we are still between 20-29 lines). I don't want to care about existing test cases when adding a new one. So please keep this file independent of line numbers, and move all your line-dependent code to a new file. For me, it should look like:
main.cpp:

int main ()  {
  [...]
  symbol_list_lines_for_inline_test();
  [...]
}

symbol_list_lines_for_inline_test.cpp:

// Skip lines so we can make sure we're not seeing any lines from x.h
[...]
// line 17
// line 18
// line 19
#include "x.h"
extern int j;
extern int gfunc(int i);
int i;
void symbol_list_lines_for_inline_test()  {
    i = gfunc(j);
    i += ns::s.mfunc();
    i += ns::ifunc(i);
}

And rename x.[h,cpp] to something more informative (symbol_list_lines_for_inline_test_x.[cpp|h] would be better).

test/tools/lldb-mi/symbol/x.h
1–14 ↗(On Diff #34812)

Please fix the indentation here (for ex, use clang-format)

tools/lldb-mi/MIUtilString.cpp
69 ↗(On Diff #34812)

Remove this empty line

This revision now requires changes to proceed.Sep 15 2015, 11:32 PM
dawn added a comment.EditedSep 16 2015, 1:16 PM

Ilia, You did the initial implementation of -symbol-list-lines right? I've fixed a major bug in your code with this commit! You have -symbol-list-lines including lines from header files in the compilation unit! Give the test in this patch a try and you will see. It means you get bogus line information.

Oops! I'm sorry, I think I've misunderstood your issue. You are talking about the test case itself, not the restriction on the filename in the -symbol-list-lines code right? OK, no problem.

dawn marked 3 inline comments as done.Sep 16 2015, 2:31 PM

Ilia, thank you for your review - I think I understand what you are asking for now.

dawn updated this revision to Diff 34928.Sep 16 2015, 2:46 PM
dawn edited edge metadata.

Tests changed as requested by Ilia. Ok to commit?

ki.stfu accepted this revision.Sep 17 2015, 1:11 AM
ki.stfu edited edge metadata.

lgtm

abidh accepted this revision.Sep 17 2015, 1:36 AM
abidh edited edge metadata.
This revision is now accepted and ready to land.Sep 17 2015, 1:36 AM
This revision was automatically updated to reflect the committed changes.

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

lldb/trunk/tools/lldb-mi/MIUtilParse.h
13
dawn marked an inline comment as done.Nov 4 2015, 5:15 PM

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

Hi Chaoren,
I believe the issue with gcc was fixed for gcc a while back. Just checking to make sure you're no longer seeing the failure. Let me know otherwise.

lldb/trunk/tools/lldb-mi/MIUtilParse.h
13

This was fixed by Sean - the XCode build wasn't including the full set of include paths when building lldb-mi.

Yeah, it seems like it's been passing for gcc-4.9 for a while now. Thanks
for checking!