Community
Participate
Working Groups
Testcase (or one of them at least) is simple. 1) temporarily change/add to path, so that you have double semicolon in it. Any path like c:\;;d:\ will do. 2) run eclipse/cdt and try to start debug session 3) error message pops up, debug session can not start Routine that prints this is LaunchUtils.getGDBVersion. I think the problem might be connected with the way command line for exec is created.
I have found exactly the same thing on Windows! I don't know how the path came to have double semicolons in it - maybe an uninstaller left in a rogue semicolon in error. Its quite hard for users to track down. The normal advice with Eclipse path problems of this sort is to advise users to try accessing their toolchains on the command line, outside of Eclipse. The only problem is that Windows(tested on 8) is resilient to double semicolons in its PATH environment variable, so the command line test will succeed. Is there a possibility of making Eclipse CDT more resilient to this problem? Could the path environment used inside CDT be scanned for double semicolons and have them replaced? This problem is probably quite unlikely to occur for many users, but when it does its very debilitating and difficult to find. Can anyone suggest a quick fix, perhaps doing a simple java.lang.String.replaceAll on the PATH environment variable, when on windows; eg: PATH.replaceAll( "(;;)+" , ";" ); Where would be a good place to do this?
Created attachment 242233 [details] proposed patch Can you try attached patch on Windows? Last I tried this fixed a problem, but it is more of a workaround for some other bug.
Hi, thanks. I tried your patch but it didn't solve the issue. Stepping through org.eclipse.cdt.dsf.gdb.launching.LaunchUtils.getGDBVersion(ILaunchConfiguration) The call to this: process = ProcessFactory.getFactory().exec(cmd, getLaunchEnvironment(configuration)); throws with: java.io.IOException: Cannot run program "gdb": Launching failed even though I added some code into org.eclipse.cdt.dsf.gdb.launching.LaunchUtils.getLaunchEnvironment(ILaunchConfiguration) to remove any double semicolons. I don't understand why the exec is failing when I've given it a clean PATH env var.
Is this still a problem? Where do you set the invalid path? Looking at LaunchUtils.getGDBVersion() it calls LaunchUtils.getGDBPath() which gets the GDB path from the launch configuration Debugger tab. I don't see how there could be a double semi-colon in that area. Are talking about the PATH environment variable which in turn is used to lookup where the gdb binary named in the Debugger tab actually resides?
Yes, still there. I have just: 1) installed a fresh copy of Eclipse Luna (on 64bit Windows 8.1) 2) installed a fresh copy of MINGW 3) Added C:\;;D:\ to system PATH variable 4) Started Eclipse 5) Created ANSI hello world project. 6) Built project and tried to start a debug session. I get a message box showing this: 'Launching hello Debug' has encountered a problem. Error with command: gdb --version Details: Error with command: gdb --version Cannot run program "gdb": Launching failed As soon as I removed that additional semicolon (an restarted Eclipse) it starts debugger just fine.
(In reply to Branko Drevensek from comment #5) > Yes, still there. > > I have just: > 1) installed a fresh copy of Eclipse Luna (on 64bit Windows 8.1) > 2) installed a fresh copy of MINGW > 3) Added C:\;;D:\ to system PATH variable > 4) Started Eclipse > 5) Created ANSI hello world project. > 6) Built project and tried to start a debug session. > > I get a message box showing this: > 'Launching hello Debug' has encountered a problem. > Error with command: gdb --version > Details: > Error with command: gdb --version > Cannot run program "gdb": Launching failed > > As soon as I removed that additional semicolon (an restarted Eclipse) it > starts debugger just fine. So when we try to launch 'gdb --version' something looks it up in the PATH and does not find it when there are two semi-colons. I believe we find the PATH variable in LaunchUtils#getLaunchEnvironment() when calling CCorePlugin.getDefault().getBuildEnvironmentManager().getVariables(). I don't have Windows here so can someone check that we do see the double semi-colon after that line? You can then try to remove it and see if that fixes it.
I added the code suggested by Juhn Moule - so modifying env strings after they are returned by getLaunchEnvironment(configuration). Although the semicolon is successfully removed (and PATH in that string is now the same as when I remove semicolon from system PATH through windows settings), launch still fails. But it looks to me now an error is somewhere in native code (exec0 fails), probably in Win32ProcessEx.c.
Hi, I'm still very interested in getting a fix for this issue. My experiments a while ago suggested that even though I gave the exec a clean path (ie a path with any double semicolons removed) it still failed to find gdb. Could it be using a cached environment perhaps? cheers john p.s. Branko, nice one for writing the steps to reproduce the problem and trying it with Luna.
(In reply to Branko Drevensek from comment #7) > Although the semicolon is successfully removed (and PATH in that string is > now the same as when I remove semicolon from system PATH through windows > settings), launch still fails. But it looks to me now an error is somewhere > in native code (exec0 fails), probably in Win32ProcessEx.c. (In reply to John Moule from comment #8) > My experiments a while ago suggested that even though I gave the exec a > clean path (ie a path with any double semicolons removed) it still failed to > find gdb. Could it be using a cached environment perhaps? I would also like to see this fixed to avoid a very hard to find problem for an unlucky user. If someone can troubleshoot further to see what is happening after we remove the double semi-colon, please let us know of any findings.
Just one more important details: Adding mingw "bin" directory to PATH fixes this issue. 1) So, eclipse can find mingw installed at default location even if it's not in path. But launching gdb fails as soon as you have ;; somewhere in path. 2) If one adds mingw bin directory to path, double semicolons suddenly don't matter anymore. Launch works.
(In reply to Branko Drevensek from comment #10) > Just one more important details: > Adding mingw "bin" directory to PATH fixes this issue. > 1) So, eclipse can find mingw installed at default location even if it's not > in path. But launching gdb fails as soon as you have ;; somewhere in path. > 2) If one adds mingw bin directory to path, double semicolons suddenly don't > matter anymore. Launch works. Maybe because you added the 'bin' directory before the double semi-colon? If you add it after instead, does it still work? I would guess that PATH must already contain the bin directory for MinGw. Eclipse cannot find it without it. I must just be later in your original PATH
No, the order does not matter. Actually I noticed something else. When there is ";;" in PATH, exec0 is called with PATH: PATH=${MINGW_HOME}\bin;${MSYS_HOME}\bin;... Which surely can not work. And when there is no double semicolon path is as expected: PATH=C:\MinGW\bin;C:\MinGW\msys\1.0\bin;... The above is returned by getLaunchEnvironment.
(In reply to Branko Drevensek from comment #12) > No, the order does not matter. > > Actually I noticed something else. > > When there is ";;" in PATH, exec0 is called with PATH: > PATH=${MINGW_HOME}\bin;${MSYS_HOME}\bin;... > Which surely can not work. > > And when there is no double semicolon path is as expected: > PATH=C:\MinGW\bin;C:\MinGW\msys\1.0\bin;... > > The above is returned by getLaunchEnvironment. So is it CDT that does not convert the variable properly when there are two semi-colons, or is it the lower-level call, outside of CDT, that returns the different result?
Created attachment 245089 [details] bugfix Original regular expression prevents string like "C:\;;D:\" being recognized as string list and then it's not used properly in loop returning the environment. This fixes the issue for me (but needs review/testing).
Hi Branko, Great, your patch works for me! Thanks. The 2nd loop of getLaunchEnvironment() relies on getting a string list back for PATH, in which case an exception is thrown and ignored. This removes the need to merge the old PATH value with the new one. I was a little uneasy with this approach. I wonder if it is always the case that the 2nd value of PATH, from build_vars, will ever contain anything that the first PATH value doesn't? If so then you'll lose some PATH. cheers john
(In reply to Branko Drevensek from comment #14) > Created attachment 245089 [details] > bugfix > > Original regular expression prevents string like "C:\;;D:\" being recognized > as string list and then it's not used properly in loop returning the > environment. > This fixes the issue for me (but needs review/testing). Nice! I'm having trouble reproducing, but I'll try to get the right conditions, as this should not be dependent on Windows to reproduce. Banko, can you post this fix to Gerrit and can you sign the CLA? I won't be able to contribute your patch otherwise. You can email me if you need more info about getting those steps done.
(In reply to John Moule from comment #15) > The 2nd loop of getLaunchEnvironment() relies on getting a string list back > for PATH, in which case an exception is thrown and ignored. This removes the > need to merge the old PATH value with the new one. > > I was a little uneasy with this approach. I wonder if it is always the case > that the 2nd value of PATH, from build_vars, will ever contain anything that > the first PATH value doesn't? If so then you'll lose some PATH. I'm not sure I fully understand what you mean, but from what I understand of the code, the PATH variable will not be split into multiple values. When parsed, it is tagged as a list, but once fully processed, it is still returned as a single variable with its value being the entire list.
(In reply to Branko Drevensek from comment #12) I've been trying to fully reproduce the issue on Linux and I still can't. > Original regular expression prevents string like "C:\;;D:\" being recognized > as string list and then it's not used properly in loop returning the > environment. This part I can reproduce. I just have to create a string with two collons :: on Linux and I can see that EnvironmentVariableSupplier#isTextList() returns false. The part about it affecting PATH is still a mystery although I tried a bunch of combination. Maybe that is a Windows-specific problem. However, it sounds reasonable to me that a list in a variable can contain empty elements and therefore we should accept a double delimiter (;; or ::). Once Branko posts the patch to Gerrit, I believe we can go ahead with it. Thanks for the find!
Hi Marc, I'll try and be clearer. Sorry its longwinded. Here's a fragment of the getLaunchEnvironment() public static String[] getLaunchEnvironment(ILaunchConfiguration config) throws CoreException { ... // Environment variables and inherited vars HashMap<String, String> envMap = new HashMap<String, String>(); IEnvironmentVariable[] vars = CCorePlugin.getDefault().getBuildEnvironmentManager().getVariables(cfg, true); for (IEnvironmentVariable var : vars) envMap.put(var.getName(), var.getValue()); // Add variables from build info ICdtVariable[] build_vars = CCorePlugin.getDefault().getCdtVariableManager().getVariables(cfg); for (ICdtVariable var : build_vars) { try { // The project_classpath variable contributed by JDT is useless for running C/C++ // binaries, but it can be lethal if it has a very large value that exceeds shell // limit. See http://bugs.eclipse.org/bugs/show_bug.cgi?id=408522 if (!"project_classpath".equals(var.getName())) //$NON-NLS-1$ envMap.put(var.getName(), var.getStringValue()); } catch (CdtVariableException e) { // Some Eclipse dynamic variables can't be resolved dynamically... we don't care. } } ... There are two places where the environment is constructed from; i) CCorePlugin.getDefault().getBuildEnvironmentManager().getVariables() and ii) CCorePlugin.getDefault().getCdtVariableManager().getVariables(). The 2nd loop merges these two lists. But the merge does not happen if the env vars are string lists which is what a PATH value is; a CdtVariableException is thrown and ignored silently. I don't know enough about the different between the environment from getBuildEnvironmentManager and getCdtVariableManager. Specifically if there is any difference in the PATH variable value. Maybe there isn't and I'm worrying unnecessarily. I add to the PATH variable inside Eclipse using: EnvironmentVariableManager.fUserSupplier.createOverrideVariable( "PATH", "${eclipse_home}/mypath", IBuildEnvironmentVariable.ENVVAR_PREPEND, EnvironmentVariableManager.getDefault().getDefaultDelimiter()); When the ;; bug occurs the PATH value from getCdtVariableManager, which contains unresolved macros ${eclipse_home}/mypath, overwrites the PATH value in envMap from getBuildEnvironmentManager, because the CdtVariableException is not thrown because it doesn't recognise the PATH value as a stringlist. If you're happy that there's no differences in PATH value from the 2 different variable suppliers I'm happy with this fix. cheers john
I was also wondering if this (the way John explained it) is the way it's supposed to work. Proposed patch fixes this, but I'm not sure if this method is completely ok. I also tried to reproduce this on Linux just now (btw: java here only sees "system" path) but was not able to. I'll do some more debugging later. I'll also sign CLA & post patch to gerrit later today, if no better solution is proposed (but I first need to learn how to actively use gerrit :blush: ).
Thinking about it... sure it can not be reproduced on linux. Speaking from memory: Original path returned by CCorePlugin.getDefault().getBuildEnvironmentManager().getVariables() is correct PATH, but contains special provision for default MINGW installation directory (in form similar to C:\${MINGW_HOME}...). Second path, returned by CCorePlugin.getDefault().getCdtVariableManager().getVariables(cfg) also contains PATH, but this one is resolved (MINGW_HOME resolved) and replaces first one in that loop, but only if it's recognized as being a text list (which it is not, if it contains ;;). On linux those two paths are the same, as no special toolchain path is added, so even if you have :: in path, it is still fine.
(In reply to John Moule from comment #19) [...] > But the merge does not happen if the env vars > are string lists which is what a PATH value is; a CdtVariableException is > thrown and ignored silently. Thanks, I had not noticed this. > I don't know enough about the different between the environment from > getBuildEnvironmentManager and getCdtVariableManager. Specifically if there > is any difference in the PATH variable value. I don't either :( > When the ;; bug occurs the PATH value from getCdtVariableManager, which > contains unresolved macros ${eclipse_home}/mypath, overwrites the PATH value > in envMap from getBuildEnvironmentManager, because the CdtVariableException > is not thrown because it doesn't recognise the PATH value as a stringlist. That explains things. Makes sense. > If you're happy that there's no differences in PATH value from the 2 > different variable suppliers I'm happy with this fix. So, the normal case is that we have a single ; which makes PATH be seen as a stringlist. This case has been handled for years. Using a ;; is the rare and unusual case. If we make the ;; case behave like the ; case, I don't think we should be breaking something; and if we do, it means people were relying on a broken behaviour, instead of the expected one.. Also, if it is possible for the second PATH variable to be different than the first and also needing not to be ignored, that would be a different bug and should be handled separately. So, I'm still ok with this fix :)
(In reply to Branko Drevensek from comment #20) > I'll also sign CLA I see this was done. Thanks > & post patch to gerrit later today, if no better solution > is proposed (but I first need to learn how to actively use gerrit :blush: ). This may help: https://wiki.eclipse.org/CDT/git#Using_Gerrit_for_CDT
Thanks for the link (was helpful), but I'm still not sure I filled everything correctly (as I was not sure which parts of tutorial is valid for comitters and which for contributors). The link: https://git.eclipse.org/r/#/c/30000/
(In reply to Branko Drevensek from comment #24) > Thanks for the link (was helpful), but I'm still not sure I filled > everything correctly (as I was not sure which parts of tutorial is valid for > comitters and which for contributors). The link: > https://git.eclipse.org/r/#/c/30000/ Looks good. I will review and post comments (if necessary) on Gerrit. You're review 30,000 exactly. Cool :)
Since we've been discussing things in bugzilla all the time, I'll continue here. I've been thinking about the proposed solution and now I'm worried that this change would cause something like: MyClass::MyMethod to be considered a text list when it is not. Maybe we should only accept a double delimiter if we are already dealing with a text list? Something like: "if there is a single delimiter somewhere, then a double one will be accepted". It is not ideal but it may be safer. It would mean that "c:\;;d:\" would not work, but I don't know that that is a likely situation; we are probably more likely to have something like "c:\blabla;c:\foo\bar;;d:\foo;d:\bar". What do you guys think?
If what is stated in https://bugs.eclipse.org/bugs/show_bug.cgi?id=284843 (which is BTW original bug report for which method in question was created) is correct, default delimiter is space and : and ; is used for environment variables. Also isTextList method is private and seems to be used only for environment variables. This does mean that making this change would modify the type of environment variable containing class name into VALUE_TEXT_LIST. Your proposal sounds entirely fine for practical purposes. I'd keep isTextList as it is (with this additional + after delimiter) and add additional method/check containsSingleDelimiter that would check, if text list is non-degenerate. Does this seem reasonable?
(In reply to Branko Drevensek from comment #27) > > Your proposal sounds entirely fine for practical purposes. > I'd keep isTextList as it is (with this additional + after delimiter) and > add additional method/check containsSingleDelimiter that would check, if > text list is non-degenerate. > > Does this seem reasonable? Sounds good.
The latest proposed fix causes a JUnit test to fail. Branko can you check if the check should be updated or the fix broke something: org.eclipse.cdt.core.envvar.IEnvironmentVariableManagerTests.testBug284843 (from org.eclipse.cdt.core.suite.AutomatedIntegrationSuite) expected:<value1[]> but was:<value1[:]> Stacktrace junit.framework.ComparisonFailure: expected:<value1[]> but was:<value1[:]> at junit.framework.Assert.assertEquals(Assert.java:100) at junit.framework.Assert.assertEquals(Assert.java:107) at junit.framework.TestCase.assertEquals(TestCase.java:269) at org.eclipse.cdt.core.envvar.IEnvironmentVariableManagerTests.testBug284843(IEnvironmentVariableManagerTests.java:660)
Sure, I've seen tests fail and will look into it.
Is the fix for this still just the modification to org.eclipse.cdt.internal.core.cdtvariables.EnvironmentVariableSupplier.isTextList()'s regex attached in https://bugs.eclipse.org/bugs/show_bug.cgi?id=389823#c14 ? Or are there some additional things? cheers john
Hi John, Current version extends this a bit. You can see diff here: https://git.eclipse.org/r/#/c/30000/2/core/org.eclipse.cdt.core/src/org/eclipse/cdt/internal/core/cdtvariables/EnvironmentVariableSupplier.java At least one existing testcase fails with this change. I'll look into this later today. Either additional change(s) are required to fix or testcase needs to be modified.
Existing tests for one of previous bugs requires that "something:" is also a list. Fixed code didn't recognize it as such. Latest commit to gerrit fixes this. I also added some sanity tests for this latest change. One thing that I noticed is that original code assumes, it can throw away any empty elements (so "something:" becomes { "something" } and not { "something", "" }. New code behaves a bit weird here, as it also throws away any last empty element (so "something::" would become { "something", "" } ).
Hi Branko, thanks for the update. cheers john
Tests now pass. However: I have been looking at bug 284843 again. That regular pattern that caused c:\;;d:\ not to be recognized as string list was introduced due to ":" being removed from environment variable containing just that (":"). If original code instead processed this as { "", "" } then it would properly rebuild string back to ":" from this list and it would probably work fine for original reporter of that bug. Even after my last change to patch for this bug, some delimiters are still removed. For example: "/usr/bin:" is converted to { "/usr/bin" } while it should actually be { "/usr/bin", "" }, which in Linux means current directory is also in path. I think EnvVarOperationProcessor.convertToString((EnvVarOperationProcessor.convertToList(string, delimiter)) should always return original string. Fixing this would probably remove need for all those regular expressions added.
(In reply to Branko Drevensek from comment #35) > Tests now pass. > > However: I have been looking at bug 284843 again. That regular pattern that > caused c:\;;d:\ not to be recognized as string list was introduced due to > ":" being removed from environment variable containing just that (":"). If > original code instead processed this as { "", "" } then it would properly > rebuild string back to ":" from this list and it would probably work fine > for original reporter of that bug. > > Even after my last change to patch for this bug, some delimiters are still > removed. For example: "/usr/bin:" is converted to { "/usr/bin" } while it > should actually be { "/usr/bin", "" }, which in Linux means current > directory is also in path. > > I think > EnvVarOperationProcessor.convertToString((EnvVarOperationProcessor. > convertToList(string, delimiter)) should always return original string. > Fixing this would probably remove need for all those regular expressions > added. I'm including Marc-Andre in this conversation since he was involved in bug 284843.
Sorry if I missing some comments, because discussion is huge, I was looking at code here https://git.eclipse.org/r/#/c/30000/4 At the function that checks validity of path is something like that private static boolean isTextList(String str, String delimiter) { if (delimiter == null || "".equals(delimiter)) return false; // Regex: ([^:]+:+)+[^:]* String patternStr = "([^" + delimiter + "]+" + delimiter + "+)+[^" + delimiter + "]*"; return Pattern.matches(patternStr, str); } I pointed out that is pretty much equal to str.contains(delimiter); except it does not handle case like ":a" to be valid (And since delimiter is not escaped it won't technically work for arbitrary delimiter) So question is why we cannot remove this useless code and replace it with contains?
Yes, this is huge discussion of a tiny bug. :) To summarize: Related bug 284843 was created to fix behavior where ":" was recognized by string list. Regular expression better detect string lists was introduced as fix for that bug. This broke recognition of windows paths like "c:\;;d:\" as string list (as it requires "not separator" to follow separator), which causes gdb launch to fail under certain conditions. My original fix for this bug allowed for strings separated with multiple separators (like "this::that") to still be recognized as string list, while originally it was not. This fixed gdb failing to launch on Windows systems with path like "c:\;;d:\". Marc then suggested this change could make some variable containing for example static method name into a string list, so we I further extended this string list check into a "string list is anything containing at least one single colon and any number of multiple colons". This is quite a mess that I think might be better solved by modifying EnvVarOperationProcessor (see comment 35). I hope someone can suggest which way to proceed. If this was fixed, I think all those regular expression code could go away. (BTW: One difference between contains and that regular expression is regular expression does not recognize ":" to be a string list.)
What is the history here? How about we forget on how it used to not work and define what is suppose to be? If we talking about variables which are path lists, i.e. separated by os separator (: on linux) and (; on windows)? If so, a) empty string is valid element of the list so ":" is valid b) if separator is escaped, it should be not considered i.e. "a\:b" is not a list (I don't know how windows escapes stuff, but not with \ since that is file separator). I.e. on linux I can create a file like this touch "a:b" ls -l a* -rw-rw-r-- 1 elaskavaia elaskavaia 0 Aug 7 12:47 a:b
(In reply to Branko Drevensek from comment #0) > Testcase (or one of them at least) is simple. > > 1) temporarily change/add to path, so that you have double semicolon in it. > Any path like c:\;;d:\ will do. > 2) run eclipse/cdt and try to start debug session > 3) error message pops up, debug session can not start > > Routine that prints this is LaunchUtils.getGDBVersion. > > I think the problem might be connected with the way command line for exec is > created. I experienced quite similar issues with Mars.1 Release (4.5.1), but in my case the Windows path is clean (No ";;" in the system path). Eclipse introduced the ";;" itself in its own CYGWIN project environment path when generating it from the Windows path. For more details on what I experienced,see https://www.eclipse.org/forums/index.php/t/1083143/
