Bugzilla – Full Text Bug Listing
| Summary: | Unable to debug when GDB command contains additional arguments | ||
|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | William Riley <william.riley> |
| Component: | cdt-debug-dsf-gdb | Assignee: | Marc Khouzam <marc.khouzam> |
| Status: | RESOLVED FIXED | QA Contact: | Marc Khouzam <marc.khouzam> |
| Severity: | critical | ||
| Priority: | P3 | CC: | cdtdoug, elaskavaia.cdt, iulia.vasii, mober.at+eclipse, pawel.1.piech, teodor.madan |
| Version: | 8.6.0 | ||
| Target Milestone: | 8.6.0 | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| Whiteboard: | |||
| Bug Depends on: | |||
| Bug Blocks: | 510615 | ||
|
Description
William Riley
Looks like this a result of https://bugs.eclipse.org/bugs/show_bug.cgi?id=445360. Can you give me an example to reproduce the problem? (In reply to Marc Khouzam from comment #2) > Can you give me an example to reproduce the problem? The 2 cases I have issues with are both architecture specific arguments, but I just tried and "gdb --silent" should be enough to reproduce it. Yeah, I can see it too. I'll look into it. This bug could be tricky. With bug 445360, we allowed the path to the gdb binary to have spaces. Teo, Iulia and Elena were involved in that effort, which is why I'm copying them. We effectively did that by taking the entire string specified by the user in the launch configuration under "GDB Debugger" and treating it as the binary. So using /home/my user/bin/gdb now works because the entire string is used to launch gdb. While before, it would only use /home/my. However, as William reports, this change also implies that using gdb --silent will fail, as the binary we try to use is "gdb --silent". Another case that now fails is ssh localhost gdb which I believe was a usecase Martin cares about, which is why I copied him. I believe we can break down the problematic cases into two aspects: A- path with spaces e.g., /home/my user/gdb B- spaces in arguments e.g., gdb --silent Which could end up being both /home/my user/gdb --silent So, the trick is to figure out which spaces are part of the binary path and which are not. My first thoughts are: 1- base ourselves on the path delimiter (/). Anything before a path delimiter is part of the path, and anything after is not. This won't work if the arguments use the patch delimiter, e.g., ssh localhost /home/bin/gdb 2-iteratively include each space until we find a corresponding file. This will be tricky when not using a path at all for the binary: gdb --silent since we won't know where to look for "gdb", so we'll need to special case it. Also, #2 is error prone as we run the risk of having name collision, e.g., we would fail with /home/my user/gdb if the file /home/my also exists. This needs some thought. If anyone has ideas, please jump in. One idea to handle using same escaping rule as most shell do, i.e. treat strings enclosed in double quote as single arg. This is generally available on both Linux and Windows hosts. Also escape rules for double quote should be handled. Another rule would be also to handle character escaping rules usually part of *unix shells, but we would end in entering non portable rules. For the examples, the correct way to translate to Process.exec "/home/my user/gdb" --silent --> [/home/my user/gdb][--silent] ssh localhost /home/bin/gdb --> [ssh][localhost][/home/bin/gdb] ssh localhost "/home/my user/gdb" --> [ssh][localhost][/home/my user/gdb] ssh localhost /folder_withquotes/\"bla/gdb --> [ssh][localhost][/folder_withquotes/\"bla/gdb] (In reply to Teodor Madan from comment #7) > One idea to handle using same escaping rule as most shell do, i.e. treat > strings enclosed in double quote as single arg. This is generally available > on both Linux and Windows hosts. Also escape rules for double quote should > be handled. > > Another rule would be also to handle character escaping rules usually part > of *unix shells, but we would end in entering non portable rules. > > For the examples, the correct way to translate to Process.exec > "/home/my user/gdb" --silent --> [/home/my user/gdb][--silent] > ssh localhost /home/bin/gdb --> [ssh][localhost][/home/bin/gdb] > ssh localhost "/home/my user/gdb" --> [ssh][localhost][/home/my user/gdb] > ssh localhost /folder_withquotes/\"bla/gdb --> > [ssh][localhost][/folder_withquotes/\"bla/gdb] I think this would be more understandable than some custom rules which might sometimes pick the wrong part of the path. It would also allow for any part of the command to contain spaces, not just the 1st part (e.g. ssh localhost "/home/my user/bin/gdb"). These are also the rules used when parsing process arguments. I agree that we should threat gdb command that user set in UI as shell command line, so we apply stanard shell arguments splitting logic, then call correct API latter on already split arguments. API itself should not go any splitting (unless we want to be backward compatible and we need another clean API for just gdb path) So we have to modify UI on that tab - if they click browse and selected gdb path that contain spaces it has to be quoted on that line I also like this idea. I just don't want to write a shell parser :) Is there such a feature already available in Java? I will look around for it, but if someone knows, please mention it. (In reply to Marc Khouzam from comment #10) > I also like this idea. I just don't want to write a shell parser :) Is > there such a feature already available in Java? I will look around for it, > but if someone knows, please mention it. There is a utility class, CommandLineUtil, in org.eclipse.cdt.utils that is already used in a few places. I'll do my best to have a fix before RC2. However, depending on the complexity of it, it may be too risky for inclusion this late. If we find it too risky (or if it is not ready), I would suggest reverting bug 445360 for 8.6. We know that the code before 445360 was running safely, so that reverting is less risky. I think not accepting paths with spaces (bug 445360) is not as big a problem as not accepting arguments to gdb (this bug). Does that make sense to others? (In reply to Marc Khouzam from comment #12) > I'll do my best to have a fix before RC2. > However, depending on the complexity of it, it may be too risky for > inclusion this late. > > If we find it too risky (or if it is not ready), I would suggest reverting > bug 445360 for 8.6. We know that the code before 445360 was running safely, > so that reverting is less risky. Though I would not revert completely (i.e. API change as well). Just calls to Process.exec. I think I can have a look at this for the backup plan for this weekend. >I think not accepting paths with spaces > (bug 445360) is not as big a problem as not accepting arguments to gdb (this > bug). > > Does that make sense to others? Hi, I'm ok with reverting bug 445360 for 8.6 and come up with a better fix later. However, tomorrow I will work on this to find a fix. It turned out to be pretty easy to implement. Here is a preliminary patch: https://git.eclipse.org/r/40798 I may clean it up a bit more, but the solution shouldn't change. Can people try it? It worked for my preliminary tests. I tried with "/home/lmckhou/gdb test/gdb" "/home/lmckhou/gdb test/gdb" --silent in all our different launch configuration types and it always works. I committed to master after Elena's review. I think this is safe enough for cdt_8_6 so I cherry picked it to review: https://git.eclipse.org/r/40803 Please confirm if you agree. Thanks William for reporting it and pointing me to CommandLineUtil. And thanks Teo for the idea of parsing like the shell. And thanks Elena for the review and input. I like these team efforts :) I've opened bug 458860 as an enhancement to follow this same parsing pattern for the path to gdbserver in our Automatic Remote launch. Marc, I'm glad you could sort this out so nicely. I must thank you all for the time and effort you have put into this. (In reply to Marc Khouzam from comment #16) > I tried with > > "/home/lmckhou/gdb test/gdb" > "/home/lmckhou/gdb test/gdb" --silent > > in all our different launch configuration types and it always works. > I committed to master after Elena's review. > > I think this is safe enough for cdt_8_6 so I cherry picked it to review: > https://git.eclipse.org/r/40803 > > Please confirm if you agree. > > Thanks William for reporting it and pointing me to CommandLineUtil. > And thanks Teo for the idea of parsing like the shell. > And thanks Elena for the review and input. > I like these team efforts :) I tried and can confirm this fixes my original cases. Thanks for the fast fix. I also checked and the original behavior of paths with spaces on Windows is unaffected (working without quotes), although quotes in the path also work now. (In reply to William Riley from comment #19) > I tried and can confirm this fixes my original cases. Great! > I also checked and the original behavior of paths with spaces on Windows is > unaffected (working without quotes) That was really important. I can't test on Windows. So we haven't broken anything too obvious on Windows. Elena or Teo, do you agree to put this in to 8_6? (In reply to Marc Khouzam from comment #20) > That was really important. I can't test on Windows. So we haven't broken > anything too obvious on Windows. > > Elena or Teo, do you agree to put this in to 8_6? Validated on Windows as well. Works. I agree to push for CDT 8_6 as well. (In reply to Teodor Madan from comment #21) > Validated on Windows as well. Works. I agree to push for CDT 8_6 as well. Great! Thanks. Committed to 8_6. Thanks everyone! |