[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Jonathon Penix via Phabricator via cfe-commits]

jpenix-quic marked 2 inline comments as done.
jpenix-quic added inline comments.


================
Comment at: flang/runtime/environment.cpp:77
+#else
+  envp = environ;
+#endif
----------------
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?


================
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>>>>>>
----------------
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!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130513/new/

https://reviews.llvm.org/D130513