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

[Jonathon Penix via Phabricator via cfe-commits]

jpenix-quic added inline comments.


================
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:
> > > 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.
The issue I hit with the suggested approach is that in order to use the pre-existing runtime environment variable handling to set the internal state I need to set the environment variable defaults before the environment variables are read by the runtime.

I might be misunderstanding/missing something, but given that the environment variables are read as part of `RTNAME(ProgramStart)` in `main` and the earliest I can place the call if I am generating it is `_QQmain`, I think that leaves three options: 1. don't hardcode `main` so that I can place the call early enough 2. delay or rerun the code [[ https://github.com/llvm/llvm-project/blob/c619d4f840dcba54751ff8c5aaafce0f173a4ad5/flang/runtime/environment.cpp#L50-L90 | here ]] that is responsible for initializing the runtime state so that it is called as part of `_QQmain` so I can insert my runtime function before it or 3. hardcode something like the `_QQEnvironmentDefaults` into Fortran_main.c so that the environment defaults are available early enough. Option 2 seems less than ideal to me, option 1 seems ideal but requires generating `main`, so option 3 is what I ended up going with. If options 1 or 2 would be preferable to what is currently implemented (or if there is another possibility I'm missing!) I'd be happy to switch and try to implement them. 



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

https://reviews.llvm.org/D130513