ExpressionParser: inherit the environment for the REPL

[compnerd]

This is particularly important on Windows where Path needs to be propagated to the inferior to allow LoadLibraryW(L"swiftCore.dll") to succeed. The library is looked up via the Path environment variable and the default target environment is empty. As a result, the library is not found and the inferior exits terminating the REPL instance.

Fixes: swiftlang/swift#76702
Fixes: swiftlang/swift#70005

[compnerd]

CC: @adrian-prantl @JDevlieghere @bulbazord

[compnerd]

@swift-ci please test

[kastiglione]

My one concern is: what issues will this have by inheriting the full environment? Either on windows or any other platform? Should this be smarter, possibly by inheriting only certain env variables (perhaps by platform)?

[compnerd]

@kastiglione you are mirroring my concern and why I'm somewhat hesitant with making this change. On the other hand, I do agree with @al45tair that it is odd for the REPL to not have the full environment.

[kastiglione]

that's a fair point

[compnerd]

@swift-ci please test macOS platform

[compnerd]

@swift-ci please test macOS platform

[compnerd]

Can't seem to reproduce the test failure locally :/

[compnerd]

@swift-ci please test macOS platform

[AnthonyLatsis]

@adrian-prantl Since this issue and llvm#70005 are being fixed in the LLVM repo, could you please transfer them?

[adrian-prantl]

@adrian-prantl Since this issue and llvm#70005 are being fixed in the LLVM repo, could you please transfer them?

Sorry I'm confused what you mean: transfer what to where?

[adrian-prantl]

@compnerd It sounds like that should be testable in a Shell repl test by running env FOO=1 lldb ... and calling getenv from withing the REPL and FileChecking the result.

I also agree with @kastiglione : What do you think about allow-listing the environment settings you need?

[compnerd]

@adrian-prantl would we also whitelist the random testing environment variable? I think that we should be allowing all the environment variables really - why would the REPL which is launched from the shell not have its environment variables? I may be doing something like GIVE_ME_MY_ENVIRONMENT_VARIABLE=YES swift repl and then expecting to be able to use it.

[jimingham]

I would expect:

lldb -O "settings set target.env-vars FOO=bar" --repl

would make a REPL in which there is an environment variable FOO that has the value BAR. I don't think this will be true if you just use the inherited environment. So I think it is formally wrong to just swap out the target one for the inherited one.

If we need to provide certain environment variables to the REPL for it to work, I think you need to add them to the target environment.

[adrian-prantl]

Who is setting these environment variables in the first place? I'm also wondering if the swift driver (invoked as swift repl) should be launching lldb using the flag @jimingham mentioned instead.

[al45tair]

Who is setting these environment variables in the first place?

Anyone, but generally the user, the operating system, installers that modify the default environment variables. Perhaps it would help to contrast with Python:

$ python
Python 3.9.6 (default, Aug  9 2024, 14:24:13) 
[Clang 16.0.0 (clang-1600.0.26.3)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.environ["HOME"]
'/Users/alastair'

but

$ swift repl
Welcome to Apple Swift version 6.0 (swiftlang-6.0.0.9.10 clang-1600.0.26.2).
Type :help for assistance.
  1> import Foundation
  2> ProcessInfo.processInfo.environment["HOME"]
$R0: String? = nil

I don't think the notion that the target has a separate environment that isn't inherited is very useful for the repl. If users want to set extra environment variables for the repl, they'll do so like this:

$ MY_VARIABLE=foobar swift repl

and not by passing extra arguments to lldb.

[al45tair]

This isn't just a Windows problem, I might add. It's weird on Darwin, never mind Windows. It's just on Windows it also breaks things in a really obvious manner, because dynamic library loading relies on Path. On Linux and Darwin, you likely won't get link errors, but things like TERM, LANG and HOME are going to be a problem.

[al45tair]

I would expect:

lldb -O "settings set target.env-vars FOO=bar" --repl

would make a REPL in which there is an environment variable FOO that has the value BAR. I don't think this will be true if you just use the inherited environment. So I think it is formally wrong to just swap out the target one for the inherited one.

If we need to provide certain environment variables to the REPL for it to work, I think you need to add them to the target environment.

I think we want to provide all the environment variables to the REPL. The fact that REPL happens to be LLDB under the covers is an implementation detail, and I don't think swift-driver should be passing the environment through by adding commands to the LLDB command line. It may be that this PR is wrong and that --repl should cause LLDB to initialise the target environment from the inherited environment (which would make the -O "settings set target.env-vars FOO=bar" above work), but fundamentally the REPL should have access to the environment in which swift repl was executed.

[AnthonyLatsis]

Sorry I'm confused what you mean: transfer what to where?

Sorry, I meant to post that in swiftlang/swift#76702 (transfer these two issues to llvm-project). Wrong browser tab.

[jimingham]

On Oct 4, 2024, at 3:19 AM, Alastair Houghton ***@***.***> wrote: I would expect: lldb -O "settings set target.env-vars FOO=bar" --repl would make a REPL in which there is an environment variable FOO that has the value BAR. I don't think this will be true if you just use the inherited environment. So I think it is formally wrong to just swap out the target one for the inherited one. If we need to provide certain environment variables to the REPL for it to work, I think you need to add them to the target environment. I think we want to provide all the environment variables to the REPL. The fact that REPL happens to be LLDB under the covers is an implementation detail, and I don't think swift-driver should be passing the environment through by adding commands to the LLDB command line. It may be that this PR is wrong and that --repl should cause LLDB to initialise the target environment from the inherited environment (which would make the -O "settings set target.env-vars FOO=bar" above work), but fundamentally the REPL should have access to the environment in which swift repl was executed.

I'm not sure why you assert this as a general rule. Why, for instance, should the equivalent of DYLD_LIBRARY_PATH necessarily be shared - i.e. why should lldb & the REPL victim program necessarily load the same shared libraries. lldb can also debug programs that don't have the same architecture, so you could, on macOS, launch an x86_64 victim program, and connect an arm64 lldb to it. It doesn't seem to me those two programs necessarily have the same environment either. It's fine to have a way to easily say "I want to run the REPL with the same environment that my lldb has", but I don't see this having the force of necessity. Jim

…

— Reply to this email directly, view it on GitHub <#9334 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVWYMLJQ6OHDPAB63RCDZZZTSBAVCNFSM6AAAAABO3HOIR6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJTGM3DENJQGY>. You are receiving this because you were mentioned.

[al45tair]

I'm not sure why you assert this as a general rule. Why, for instance, should the equivalent of DYLD_LIBRARY_PATH necessarily be shared - i.e. why should lldb & the REPL victim program necessarily load the same shared libraries. lldb can also debug programs that don't have the same architecture, so you could, on macOS, launch an x86_64 victim program, and connect an arm64 lldb to it. It doesn't seem to me those two programs necessarily have the same environment either. It's fine to have a way to easily say "I want to run the REPL with the same environment that my lldb has", but I don't see this having the force of necessity. Jim

That only makes sense if you think of the REPL as LLDB, but users of Swift don't see it that way; what they see is swift repl doesn't behave the same way as python, php -a, node, gore etcetera.

People using swift repl are not trying to debug a program (let alone remotely). They just want somewhere they can type Swift code and have it evaluated. If that somewhere doesn't see the same environment variables as the shell they started the repl from, it seems odd and broken.

[al45tair]

To take the specific example of DYLD_LIBRARY_PATH, yes absolutely if I do

$ DYLD_LIBRARY_PATH=/foo/bar swift repl
Welcome to Apple Swift version 6.0 (swiftlang-6.0.0.9.10 clang-1600.0.26.2).
Type :help for assistance.
  1> import Darwin
  2> let foo = dlopen("foobar.dylib", RTLD_NOW)

I'm expecting the dynamic loader to look for foobar.dylib in /foo/bar. And as a user of the Swift REPL, I'm not really aware that I'm running LLDB under the covers. I accept, though, that if the DYLD_LIBRARY_PATH setting upsets the REPL process somehow and makes it go wrong, well, that's my problem.

[jimingham]

This doesn't argue that lldb should pass the environment through to the victim program willy nilly, does it? It just means "the swift driver, when invoking lldb to run the swift REPL, should tell lldb to pass all (or a reasonable subset) of the environment variables.) That more argues to me that lldb should not be the one to impose a policy here, but it should make it easy for the swift driver to pick the "pass all my environment vars" or whatever subset it wanted. There's no formal reason why you have to do "in order to run the REPL victim program with a set of environment variables, lldb has to run with the same set". We provide ways to specify the environment for the target program precisely because running lldb with the experimental libraries you want to load into the process, but not run lldb with them because you want a healthy debugger, is often a really handy thing to be able to do. I think you want to leave that decision up to users (including the swift driver) and not lldb. Jim

…

On Oct 7, 2024, at 10:59 AM, Alastair Houghton ***@***.***> wrote: To take the specific example of DYLD_LIBRARY_PATH, yes absolutely if I do $ DYLD_LIBRARY_PATH=/foo/bar swift repl Welcome to Apple Swift version 6.0 (swiftlang-6.0.0.9.10 clang-1600.0.26.2). Type :help for assistance. 1> import Darwin 2> let foo = dlopen("foobar.dylib", RTLD_NOW) I'm expecting the dynamic loader to look for foobar.dylib in /foo/bar. And as a user of the Swift REPL, I'm not really aware that I'm running LLDB under the covers. I accept, though, that if the DYLD_LIBRARY_PATH setting upsets the REPL process somehow and makes it go wrong, well, that's my problem. — Reply to this email directly, view it on GitHub <#9334 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW65JUY7M6KEPO5K343Z2LDWJAVCNFSM6AAAAABO3HOIR6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJXGU2TIMZTHE>. You are receiving this because you were mentioned.

[al45tair]

There's no formal reason why you have to do "in order to run the REPL victim program with a set of environment variables, lldb has to run with the same set".

That's true, there's no fundamental reason we have to run LLDB with the same set. But… doing anything else necessitates filtering out environment variables that might break LLDB, and then somehow passing them to LLDB so that they are inherited by the target process. That isn't trivial.

Are we suggesting, for instance, that the driver should somehow know how to filter Path on Windows to remove things that might break LLDB? Because Windows requires Path to be set correctly in order to load DLLs, including potentially ones that LLDB itself depends upon; without it, we might not even get as far as running LLDB in the first place. But with it, who is to say that some item in Path won't break LLDB?

And it's not like the REPL gains much from doing this; you can still trivially break it with the same variables you're worrying about by setting them in such a way that the driver itself won't work, so we can't say to the user that setting "dangerous" environment variables is safe, because it isn't.

[adrian-prantl]

This was also what I was getting at with my earlier question:

• should LLDB in REPL mode inherit the environment, or
• should the Swift driver set the environment of the inferior when launching LLDB as swift repl?

[al45tair]

This was also what I was getting at with my earlier question:

• should LLDB in REPL mode inherit the environment, or
• should the Swift driver set the environment of the inferior when launching LLDB as swift repl?

The most important thing is that the environment in which swift repl is run is available to the inferior process.

In principle it doesn't matter so much if the hidden LLDB itself has a different environment. However, if you're arguing that the driver should provide LLDB with a different environment, for instance by filtering out DYLD_LIBRARY_PATH, it's unclear to me what that actually buys us in terms of robustness. By the time the driver is in a position to do something like that, it has already been affected by the environment variables in question, and on Windows, the equivalent environment variable is Path, which has to be set for anything to work properly, so not passing it would just break everything.

Additionally, I worry that doing anything other than just leaving the environment alone is more complicated than it sounds; for instance, we might end up having to escape things if we're going to generate LLDB commands to set environment variables.

[jimingham]

lldb is the lower-level tool, it should be expected to be able to set up the REPL environment however the lldb user wants to do, and in fact lldb already has ways to tell it to inherit the full environment, or any subset the user chooses. The swift REPL has a particular use of lldb in mind, which it has the tools to express. So it seems more appropriate to me that it should be the one doing that.

On Oct 8, 2024, at 4:37 AM, Alastair Houghton ***@***.***> wrote: There's no formal reason why you have to do "in order to run the REPL victim program with a set of environment variables, lldb has to run with the same set". That's true, there's no fundamental reason we have to run LLDB with the same set. But… doing anything else necessitates filtering out environment variables that might break LLDB, and then somehow passing them to LLDB so that they are inherited by the target process. That isn't trivial. Are we suggesting, for instance, that the driver should somehow know how to filter Path on Windows to remove things that might break LLDB? Because Windows requires Path to be set correctly in order to load DLLs, including potentially ones that LLDB itself depends upon; without it, we might not even get as far as running LLDB in the first place. But with it, who is to say that some item in Path won't break LLDB?

The swift driver is the one to know that its intents for people using the Swift REPL are that they won't be doing anything that might get them into trouble, and so don't need a way to work around that. Its aim is not to deal with more complex development environments that might benefit from finer grained control. If that's an accurate assessment, then the swift driver can explicitly tell lldb it should "inherit the whole environment" - which you can do by passing `-O "settings set target.inherit-env 1"` in the launch line for lldb. That will work for most cases, and when there's something special that needs to be done, the user can look at the lldb command the swift driver invoked, remove that setting, add what's appropriate, and REPL on happily. Jim

…

And it's not like the REPL gains much from doing this; you can still trivially break it with the same variables you're worrying about by setting them in such a way that the driver itself won't work, so we can't say to the user that setting "dangerous" environment variables is safe, because it isn't. — Reply to this email directly, view it on GitHub <#9334 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVWYIPUGIB2X4VEYZC53Z2O7XRAVCNFSM6AAAAABO3HOIR6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJZGYYDENRRHA>. You are receiving this because you were mentioned.

[compnerd]

Note that in addition to Path, I believe that UserProfile and TEMP should be set up properly. In order to get Foundation to work, there are a few other variables as well. I think that SDKROOT also needs to be setup properly in some cases, and PROCESSOR_ARCHITECTURE is often consulted. I think that ComSpec is another one which can be used for process management, and so this is important to keep in the filter set. I imagine that the more I think about it, the longer the list will get.

[al45tair]

If that's an accurate assessment, then the swift driver can explicitly tell lldb it should "inherit the whole environment" - which you can do by passing -O "settings set target.inherit-env 1" in the launch line for lldb.

OK, that may be the thing that we need here, in which case that needs a driver change, which is fair.

[jimingham]

Note, at present the REPL has hand-rolled launch code for the victim program that always passes a default constructed ProcessLaunchInfo. The default constructed object has an empty environment. So there's no way to give any environment to the victim at present. We should fix that by making a ProcessLaunchInfo constructor that takes that Target it will be used for, and that constructor can copy over the environment, obeying all the settings. Jim

…

On Oct 8, 2024, at 10:16 AM, Alastair Houghton ***@***.***> wrote: If that's an accurate assessment, then the swift driver can explicitly tell lldb it should "inherit the whole environment" - which you can do by passing -O "settings set target.inherit-env 1" in the launch line for lldb. OK, that may be the thing that we need here, in which case that needs a driver change, which is fair. — Reply to this email directly, view it on GitHub <#9334 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW4A4B34M4ACWCGAG6DZ2QHNXAVCNFSM6AAAAABO3HOIR6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBQGQYTMMJSG4>. You are receiving this because you were mentioned.

[al45tair]

If I understand @jimingham correctly, the LLDB team has a preference for fixing #9551 and then we can update the Swift Driver to pass -O "settings set target.inherit-environment 1".