Flang compiled code returns incorrect length for a string

[kiranchandramohan]

Flang compiled code returns incorrect length for a variable-length string returned from a function.

The following code return length as 0 or a random number with Flang. Gfortran compiled code correctly returns length as 2.

program main
real, dimension(2) :: arr = 1
integer:: length
length = len(get_string(arr))
print *, length
contains
 function get_string(marg) result(res_string)
   real, dimension(:), intent(in) :: marg
   character(len=size(marg(:))) :: res_string
 end function get_string
end program main

Note: If the reference to the array is changed to just its name only (in line 9) then the generated code works fine. i.e size(marg(:)) to size(marg).

[kiranchandramohan]

The statement descriptor for calculating the length of the string looks like

z_s_0$len = max(size(xa(1_8:z_b_1),pghpf_0(3)),0_8)

z_b_1 has not been set to any value though. And this carries all the way to the LLVM IR. Since z_b_1 has not been set to any value it either picks up a random number or 0.

Not clear to me where the value of z_b_1 should be set.

[RichBarton-Arm]

@kiranchandramohan had a little look at this internally. Here are his notes: Minimal reproducer

program main
real, dimension(2) :: xa = 1
integer:: length
length = len(zfunc(xa))
print *, length
contains
 function zfunc(marg) result(res_string)
 real, dimension(:), intent(in) :: marg
 character(len=size(marg(:))) :: res_string
 end function zfunc
end program main

Here zfunc is returning a string whose length is dependent on the argument marg. This is called an adjustable length character. The length of the string is dependent on the size of the array argument 'marg'. marg's lower bound is 1 and upper bound is z_b_1. The length of the string returned is max(z_b_1,0). Unfortunately, the value z_b_1 is not loaded anywhere and hence z_b_1 takes the value of 0 or a random value. Question is which code should be setting up that load?

Today I spent most of my time chasing where the types are created, particularly the one which we are interested in.

dtype:64 ty:character
len:23=size(marg(1_8:z_b_1),pghpf_0(3))
type:64:character*??

This is created in get_type in dtypeutl.c. The get_type function is called by mod_type which creates a new type from an existing type by adding more information.

Interesting functions,

fix_character_length:  /* we have a character datatype, replace any formal arguments in

    the character length by their values, rewrite the length */

Following two functions create the result value of functions

gen_char_result 
gen_array_result

[michalpasztamobica]

I can see that there is a special entry for "adjustable length character" in is_deferlenchar_ast():

  /* adjustable length character */
  if ((sym > NOSYM) && ADJLENG(sym)) {
    return false;
  }

which gets triggered for this reproducer:

is_deferlenchar_ast() at dtypeutl.c:2,433 0x55555567eb1e	
gen_alloc_dealloc() at semutil2.c:11,532 0x5555559a3480	
allocate_temp() at semutil2.c:7,508 0x5555559946d9	
get_ch_temp() at semutil2.c:7,739 0x555555995631	
gen_char_result() at semfunc.c:2,443 0x5555558de783	
func_call2() at semfunc.c:1,153 0x5555558d7ff2	
func_call() at semfunc.c:1,482 0x5555558d9d40	
mkvarref() at semutil.c:2,022 0x55555595d9cc	
semant2() at semant2.c:1,247 0x555555880d2a	
_parser() at parser.c:302 0x5555557d0e17	
parser() at parser.c:152 0x5555557d0828	
main() at main.c:208 0x555555793a30	

It prevents len_stmt assignment in gen_alloc_dealloc(), but it refers z_s_0, rather than z_b_1... I understand z_b_X are for describing array's bounds. Could z_s_X describe array's size? Then we can infer z_b_1 as lower_bound + z_s_0. There's also this internal error from redimension being thrown, but it doesn't cause the compilation to fail as it is internal only. It highlights that marg (sptr=670) was an unused temporary, which makes sense in the context of this issue.

[michalpasztamobica]

I have compared the execution of code for the original reproducer:

character(len=size(marg(:))) :: res_string

and for its version with the name only passed to size(), which works correctly:

character(len=size(marg)) :: res_string

During sematic analysis we have a different sequence of reductions for sptr pointing to marg in line 9. In the correct case we have: PRIMARY1 -> EXPRESSION1 -> SSA1 -> SSA_LIST2 -> VAR_REF3 In the incorrect case it is: EXPRESSION2 -> VAR_PRIMARY_SSA1 -> OPT_SUB1 -> OPT_STRIDE1 -> SSA3 -> SSA_LIST2 -> VAR_REF3

Then the VAR_REF3 acts different in each case. In the correct case we hit this if, recognizing the type as TY_ARRAY, while the incorrect case has this type set to TY_REAL and will enter the else statement. Ultimately both cases enter mkvarref() function, but they will get a different sptr in this line. The correct case will get a pointer to size (STYPE = ST_PD). (ST_PD is "Predeclared subroutine name.") The incorrect case will get a pointer to marg (STYPE = ST_ARRAY).

Next the incorrect case triggers the same order as correct case did previously, but it is now doing it for the sptr pointing at size. This causes it to get a NULL pointer in this line of ref_pd(), while the correct case get a pointer to marg. This doesn't seem to infer much consequences, because marg is not a pointer and nothing special gets done to it, but clearly - the processed AST makes a differentce.

[michalpasztamobica]

I have stepped through the rest of the semantic analysis for line 9 and 10:

character(len=size(marg(:))) :: res_string
print *,len(res_string)

and also for the second pass for lines 4 and 5:

length = len(zfunc(xa))
print *, length

The most important findings are quite disappointing, but I found more differences in how two cases are being handled:

1. The "correct case" (the one which skips "(:)") creates some kind of temporary variable (sptr=637, name " ¬ÕUUU") and uses it when processing the initialization of a variable in line 10. The failing case does a much more reasonable thing as it directly points at sptr=630, name "res_string". This leads to slightly different behavior when semantic analysis is around ENTITY_ID1 and ENTITY_DECL1 reductions.
2. When processing the actual value returned by the function I can see that the correct case uses sptr=630, name "res_string" while the incorrect one chooses sptr=470 (len) - so it is kind of using the return of the len function, rather using the variable... This is a bit weird, but should still yield correct results.
3. When processing line 4 (second pass) both cases will use the res_string sptr for VAR_REF3, deciding what to assign to length variable.
4. I also checked how semantio goes through line 5 (which prints the lenght variable). In both cases new const sptr is created with the values of CONVAL1P/2P set to 0.

I am afraid that the bug might actually be in runtime...

[michalpasztamobica]

As mentioned earlier marg(:) is recognized as ST_ARRAY while marg is an ST_REAL. Qualifying that stack entry 629 "marg" as an ST_ARRAY later leads to hitting this line setting the SST_ID to S_LVALUE. Later we're processing the sptr 426 "size" in ref_pd and the top of the stack is exactly this 629 "marg". That's what will be passed to mkarg() in this line. And so mkarg() will enter the S_LVALUE case, instead of the S_IDENT case, as the marg-without-colon version of our program does. Not sure yet how this influences the further flow of the program, I will try to investigate this tomorrow.

[michalpasztamobica]

Following advice from @pawosm-arm I looked into LLVM IR.

The main difference between the correct (marg) and incorrect (marg(:)) IRs is that the incorrect takes the z_b_1 into account, while the correct one doesn't. Instead of a complicated calculation like this:

 61 L.LB1_343:                                        ; preds = %L.entry
 62   %11 = load i64, i64* %z_b_1_301, align 8, !dbg !8
 63   %12 = icmp sge i64 %11, 1, !dbg !8
 64   %13 = sext i1 %12 to i32, !dbg !8
 65   %14 = trunc i32 %13 to i1, !dbg !8
 66   %15 = load i64, i64* %z_b_1_301, align 8, !dbg !8
 67   %16 = select i1 %14, i64 %15, i64 0, !dbg !8
 68   %17 = trunc i64 %16 to i32, !dbg !8
 69   %18 = sext i32 %17 to i64, !dbg !8
 70   %19 = icmp sgt i64 0, %18, !dbg !8
 71   %20 = select i1 %19, i64 0, i64 %18, !dbg !8
 72   store i64 %20, i64* %"z_s_0$len_307", align 8, !dbg !8

the correct case simple stores "2" into the z_s_0%len_307, where 2 is exactly the length of the string:

 60 L.LB1_343:                                        ; preds = %L.entry
 61   store i64 2, i64* %"z_s_0$len_307", align 8, !dbg !8

The zfunc itself looks exactly the same for both cases and uses z_s_0$len, not z_b_1. This is inline with the difference in semantic analysis which becomes unnecessarily complicated if we use marg(:) notation. I was previously looking into gram.txt to check if perhaps the analysis is going wrong due to the extra (:), but I could not find an error in there. I will look into the parser to see how this piece of code is being tokenized, etc.

[michalpasztamobica]

@pawosm-arm has given me another short advice on how he has once checked a similar issue (store missing between alloca and load in LLVM IR for an array bound). This had to do with a missing call to mk_extent_expr. However the function gets properly called in our case and acts exactly the same for both the correct (marg) and incorrect (marg(:)) case.

I also tried to figure out why does ADJLEN(P/G) never get set to true. marg is in fact an adjustable length character, yet this macro returns false when marg is analyzed. I did not find the definitions of this macro anywhere in the source code and config files and I also did not find where it should get set, but even when I forced the code to execute what would be executed if ADJLENG returned 1, it did not make any difference and the size was still unset.

[kiranchandramohan]

Did you mean the ADJLEN macros? In the build directory. tools/flang1/utils/symtab/symtab.h:#define ADJLENG(s) (stb.stg_base[s].f52) tools/flang1/utils/symtab/symtab.h:#define ADJLENP(s,v) (stb.stg_base[s].f52 = (v))

[michalpasztamobica]

Thanks, @kiranchandramohan , yes, I meant those. Thanks for pointing them to me. It turned out my IDE required a Refresh of the build directory and then I found them as well :) Unfortunately, as I wrote - even forcing this to 1 does not fix the issue, so probably this is a dead end.

I am now looking into another potential problem. For the incorrect case (marg(:)), when processing marg we recognize it as an TY_ARRAY which leads to storing its upper bound and stride in a triple_flag with SST_DIMFLAGP in this line. There is a lengthy comment before this code section explaining how the bounds and stride would be used in runtime. I checked that SST_DIMFLAGG never gets called in the C code, so the only way to make use of this triple_flag that I can think of is in runtime...

[michalpasztamobica]

Going further I noticed that the z_b_1 variable is returned when further processing the triple in this line.