peixin added inline comments.
================ Comment at: flang/runtime/environment.cpp:77 +#else + envp = environ; +#endif ---------------- jpenix-quic wrote: > peixin wrote: > > What is `environ` used for? Should we try to keep as less extern variable > > as possible in runtime for security. > I think `setenv` is only required to make sure that the `environ` pointer > points to the correct copy of the environment variables. So, as long as we > are storing a copy of the environment pointer in `ExecutionEnvironment` and > potentially making changes to the environment via `setenv`, I think we need > to base it off the `environ` pointer after `setenv` has been called rather > than the `envp` from `main`. > > That said, I don't think the `envp` variable we are storing is being used for > anything at the moment (reading environment variables was changed to use > `getenv` rather than `envp` in the commit [[ > https://github.com/llvm/llvm-project/commit/824bf908194c9267f1f09065ba8e41d7969006ab > | here]]). If removing the usage of `environ` is preferable, we could maybe > remove the usage of `envp` altogether (in a separate patch)--does this sound > like a good idea or would it be better to just leave in the `environ` usage > for now? Thanks for the explanations. The current fix makes sense to me. ================ Comment at: flang/test/Driver/emit-mlir.f90:16 ! CHECK-NEXT: } +! CHECK-NEXT: fir.global @_QQEnvironmentDefaults constant : !fir.ref<tuple<i[[int_size:.*]], !fir.ref<!fir.array<0xtuple<!fir.ref<i8>, !fir.ref<i8>>>>>> { +! CHECK-NEXT: %[[VAL_0:.*]] = fir.zero_bits !fir.ref<tuple<i[[int_size]], !fir.ref<!fir.array<0xtuple<!fir.ref<i8>, !fir.ref<i8>>>>>> ---------------- jpenix-quic wrote: > peixin wrote: > > jpenix-quic wrote: > > > peixin wrote: > > > > Is it possible not to generated this global variable if `fconvert=` is > > > > not specified? > > > I'm not entirely sure--the issue I was running into was how to handle > > > this in Fortran_main.c in a way which worked for all of GCC/Clang/Visual > > > Studio (and maybe others?). I was originally thinking of doing this by > > > using a weak definition of _QQEnvironmentDefaults set to nullptr so > > > fconvert, etc. could override this definition without explicitly > > > generating the fallback case. For GCC/clang, I think I could use > > > __attribute__((weak)), but I wasn't sure how to handle this if someone > > > tried to build with Visual Studio (or maybe another toolchain). I saw a > > > few workarounds (ex: > > > https://devblogs.microsoft.com/oldnewthing/20200731-00/?p=104024) but I > > > shied away from this since it seems to be an undocumented feature (and > > > presumably only helps with Visual Studio). > > > > > > Do you know of a better or more general way I could do this? (Or, is > > > there non-weak symbol approach that might be better that I'm missing?) > > How about generate one runtime function with the argument of > > `EnvironmentDefaultList`? This will avoid this and using one extern > > variable? > > > > If users use one variable with bind C name `_QQEnvironmentDefaults` in > > fortran or one variable with name `_QQEnvironmentDefaults` in C, it is > > risky. Would using the runtime function and static variable with the type > > `EnvironmentDefaultList` in runtime be safer? > Agreed that there are potential risks with the current approach (although, > are the `_Q*` names considered reserved?). Unfortunately, I think generating > a call to set the environment defaults requires somewhat significant changes > to the runtime. The runtime reads environment variables during initialization > in `ExecutionEnvironment::Configure` which is ultimately called from the > "hardcoded" `Fortran_main.c` and I need to set the defaults before this > happens. So, I believe I'd either have to move the initialization to > `_QQmain` or make it so that `main` isn't hardcoded so that I could insert > the appropriate runtime function. > > @klausler I think I asked you about this when I was first trying to figure > out how to implement the environment defaults and you suggested I try the > extern approach--please let me know if you have thoughts/suggestions around > this! This is what @klausler suggested: ``` Instead of adding new custom APIs that let command-line options control behavior in a way that is redundant with the runtime environment, I suggest that you try a more general runtime library API by which the main program can specify a default environment variable setting, or a set of them. Then turn the command-line options into the equivalent environment settings and pass them as default settings that could be overridden by the actual environment. ``` If I understand correctly, what I am suggesting match his comments. The "main program" he means should be fortran main program, not the `RTNAME(ProgramStart`. In your initial patch, you add the runtime specified for "convert option". I think @klausler suggest you making the runtime argument more general used for a set of runtime environment variable settings, not restricted to "convert option". And that is what you already added -- `EnvironmentDefaultList`. So, combining this patch and your initial patch will be the solution. Hope I understand it correctly. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130513/new/ https://reviews.llvm.org/D130513 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
