Hi Tobias, On Mon, Oct 20, 2025 at 6:48 PM Tobias Burnus <[email protected]> wrote: > > Yuao Ma wrote: > > This patch introduces support for the NIL value within conditional > > arguments, addressing several scenarios for how NIL should be handled. > > Let's start with two testcases that fail: > > (A) Passing a string argument > > I found a test that fails with an ICE (segmentation fault) > in generic_simplify_COND_EXPR, called by gfc_conv_conditional_expr: > > implicit none > interface > subroutine sub(x) > character(*), optional :: x > end > end interface > logical :: cc > call sub( (cc ? "132" : .nil.)) > end > > I have not checked, but I assume that it has a NULL_TREE > instead of passing '0' as length. I think that should be > gfc_index_zero_node but I have not checked. > (I think gfc_index_zero_node is of type signed with the > same size as 'size_t'/size_type_node - and I think that > is what gfortran uses for the hidden length ( >
My apologies. I should have included at least one test case specifically for the character type. I've now addressed this by using build_int_cst(gfc_charlen_type_node, 0). > * * * > > (B) OPTIONAL + VALUE > > In that case, a null-pointer cannot be used. GCC follows the IBM > compiler by passing a hidden argument to denote the present > status. > > The patch does not handle this case, passing always .true.: > > sub (1, D.4682 ? 1 : (void) 0, 1); > sub (0, D.4683 ? 1 : (void) 0, 1); > > for > > module m > implicit none > contains > subroutine sub(expected, x) > logical, value :: expected > integer, value, optional :: x > if (expected .neqv. present(x)) error stop > end > subroutine test > logical :: cc > cc = .true. > call sub(.true., (cc ? 1 : .nil.)) > cc = .false. > call sub(.false., (cc ? 1 : .nil.)) > end > end > > use m > call test > end > Thanks for catching that! Honestly, this section is what took up the majority of my time when I was updating the patch. Here are the main changes I made: 1. trans-expr: If we have se->want_pointer or type == BT_CHARACTER, expr is now set to null_pointer_node; otherwise, it's set to iinteger_zero_node. 2. conv_dummy_value: I'm now treating cond-expr just like a VARIABLE. I check if the final result is nullptr to see if the optional argument is absent, and I also fixed a block-related bug here. 3. The resolve logic needed an update because the type information is required for the nil argument. In the testcase I added integer optional value and character optional value. > * * * > > [Added after writing the following. > Reading through your text, I see that you already mentioned > that some features will be handled in follow-up patches, > like C1542. The latter also mentions coarrays. Thus, I think > you can leave this one out (i.e. keep the current error and > don't add a 'sorry' here), fixing the issues later.] > > > (C) I now tried scalar coarrays – and those fail as well. > > I got the idea because for them also hidden arguments are > passed on. > > The simplest way is to compile with -fcoarray=single, which > mostly ignores all coarrays - and 'this_image()' is always > one. > > If you compile with '-fcoarray=lib' actual library calls are > inserted. If you link with '-lcaf_single', that's a stub > library which permits only a single image. > > If you want to try with real coarray support, you need the > http://www.opencoarrays.org/ library - and link it instead > of libcaf_single.so, but that shouldn't be needed here and > for the code gen, it is identical to the caf_single case. > > (There is a subdirectory in testsuite/gfortran.dg/ that > handles coarray testing.) > > > module m > implicit none > integer :: my_coarray[*], my_coarray2[*] > contains > subroutine sub(expected, x) > logical, value :: expected > integer, optional :: x[*] > integer :: image > image = this_image() > if (expected .neqv. present(x)) error stop 1 > if (present(x)) then > if (x[image] /= 1) error stop 2 > end if > end > subroutine sub2(x) > integer :: x[*] > integer :: image > image = this_image() > if (x[image] /= 2) error stop 3 > end > subroutine test > logical :: cc > cc = .true. > > ! Bogus error: Actual argument to ‘x’ at (1) must be a coarray > > call sub2((cc ? my_coarray2 : my_coarray)) > > call sub(.true., (cc ? my_coarray : .nil.)) > > cc = .false. > call sub(.false., (cc ? my_coarray : .nil.)) > end > end > > use m > my_coarray = 1 > my_coarray2 = 2 > call test > end > > * * * > > NOTE: Besides the case of passing coarrays as above, one can also have > allocatable coarrays. It works mostly similar, but the token is handled > differently. Try: > > subroutine sub(x) > integer, allocatable :: x[:] > and then pass an allocatable coarray to it. > > * * * > > Can you check the cases above? I think the first two should really > work. > The updated patch will resolve the first two cases. Regarding the coarray, if it's not causing an ICE, I'd prefer to postpone working on it until we handle arrays. > * * * > > Side remark: I was wondering about the diagnostic for: > subroutine sub(x) > character(5), optional :: x > and passing a too-short string. > > Result: No message for: > call sub( (cc ? "132" : "12")) > but for > call sub( (cc ? "132" : "345")) > we get: > Warning: Character length of actual argument shorter > than of dummy argument ‘x’ (3/5) at (1) > > Thus, this part seems to work FINE :-) > > (It is possible to warn for the first case, but it is > rather special, hence, I don't expect a warning for it > by adding a specific test for it.) > Currently, our string_length underneath is also a COND_EXPR. This warning will only be triggered if the string_length is a constant (or simplified to a constant). In the second example, the string_length of both operands is the same, so we will receive the warning. However, in the first example, we will not. > * * * > > > We now support three distinct behaviors for NIL argument: > > - accept it for actual arguments to OPTIONAL dummy arguments > > - reject it for dummy arguments that are not OPTIONAL > > - reject it when not used as actual argument. > > It is also rejected when used as, e.g., '1 + (c ? var : .nil.)' > as it should. > > * * * > > > Implementation Details > > - The NIL node is represented by an expression of type > > EXPR_CONDITIONAL where the condition field is nullptr. > > - To distinguish between the valid and invalid uses mentioned above, > > we rely on the global state actual_arg to determine if the expression > > is being used as an actual argument. > > ... which permits the diagnostic mentioned above. (In case of doubt: > The patch only adds another use case for actual_arg, the global variable > is not actually new.) > > > This patch does not yet address diagnostics for intrinsic procedures. > > Handling NIL arguments for intrinsics is complex because most > > intrinsic functions already utilize their own custom argument-checking > > functions. While one option would be to add a new loop to the > > check_specific - traversing the argument list and verifying the > > OPTIONAL status - I haven't yet committed to this approach, as I'm > > unsure if it's the most optimal solution. > > Note that the use of 'optional' in intrinsic functions is also > rather limited. > Most don't permit optional arguments - and some have an optional > argument but many, it means either no argument or a present argument > and not a dummy argument that might be absent. > > Thus, while this is an omission, I think it has nearly zero effect > on real-world code. > > * * * > > > Regarding the Fortran 2023 standard, C1538 - C1545 outline additional > > requirements for conditional arguments involving NIL. I plan to > > gradually expand support to meet these extra requirements over > > subsequent patches. > > Some are handled (as they are the same as existing tests), others > are argument specific. > > On the other hand, some features are similar like: > > implicit none > integer, pointer :: p > integer, target :: x > integer, target :: y > logical :: cond > cond = .true. > p => (cond ? x : y) > end > > This fails with: > Error: Pointer assignment target is neither TARGET nor POINTER at (1) > > We can argue about the wording – but an error is correct as this > example is INVALID - pointer assignment requires either > a designator or a function reference that returns a pointer. > > Thus, when fixing the current code to handle the following (valid) > example, we still have to print an error for the code above. > > subroutine s(x) > integer, pointer :: x > ... > call s( (cond ? ptr1 : ptr2) ) ! valid > > * * * > > > --- a/gcc/fortran/expr.cc > > +++ b/gcc/fortran/expr.cc > > @@ -136,6 +136,21 @@ gfc_get_conditional_expr (locus *where, gfc_expr > > *condition, > > return e; > > } > > > > +/* Check if the condtional expression has .nil. or not */ > > + > > +bool > > +gfc_is_conditional_nil (gfc_expr *e) > > +{ > > + if (!e) > > + gcc_unreachable (); > > IMHO, that should either be 'gcc_assert (e != nullptr)' > or 'gcc_checking_assert (e != nullptr);' > > That's internally (system.h) > > #if ENABLE_ASSERT_CHECKING > #define gcc_assert(EXPR) \ > ((void)(!(EXPR) ? fancy_abort (__FILE__, __LINE__, __FUNCTION__), 0 : 0)) > #elif (GCC_VERSION >= 4005) > #define gcc_assert(EXPR) \ > ((void)(__builtin_expect (!(EXPR), 0) ? __builtin_unreachable (), 0 : 0)) > ... > > or > > #if CHECKING_P > #define gcc_checking_assert(EXPR) gcc_assert (EXPR) > #else > /* N.B.: in release build EXPR is not evaluated. */ > #define gcc_checking_assert(EXPR) ((void)(0 && (EXPR))) > I've switched to using gcc_assert. > * * * > > > + if (e->expr_type != EXPR_CONDITIONAL) > > + return false; > > + if (e->value.conditional.condition == nullptr) > > + return true; > > + return gfc_is_conditional_nil (e->value.conditional.true_expr) > > + || gfc_is_conditional_nil (e->value.conditional.false_expr); > > +} > > Looking at the last two line and at (BTW: there is a typo in 'cond(i)tional') > > /* Check if the condtional expression has .nil. or not */ > > … > > gfc_is_conditional_nil (gfc_expr *e) > > I am not really happy with the name – from the function name, I'd > expect that it checks that 'e' is .NIL. > > In really it checks whether the expression is or contains .NIL., > but it only checks the latter if 'e' is a conditional expression. > > > It is currently used to check whether an actual argument contains > a '.nil.' - to reject it if the dummy argument is not optional. > > It does not handle, e.g., '1 + (cond ? ... : .nil.)' but this is > already properly rejected (as mentioned above). > > The 'is itself .NIL.' check is in principle unintended, but as > the function is recursively called - > to handle '(c ? e1 : c2 ? e2 : .nil.)') > - the .NIL. check for 'e' itself makes algorithmically sense. > > > Back to the wording question. How about: > > /* Check whether the conditional expression contains .NIL. > (or is .NIL. itself). */ > … > gfc_conditional_expr_with_nil > > I think that's not ideal either, but should a bit clearer. > The naming issue is corrected. > * * * > > > @@ -1409,7 +1424,7 @@ simplify_conditional (gfc_expr *p, int type) > > || !gfc_simplify_expr (false_expr, type)) > > return false; > > > > - if (!gfc_is_constant_expr (condition)) > > + if (!condition || !gfc_is_constant_expr (condition)) > > return true; > > I think that's clearer as: > > if (!condition /* is .NIL. */ > || !gfc_is_constant_expr (condition)) > return true; > > because the internal representation of .NIL. is not obvious > and contrary to all other code dealing with .NIL. it is also > not obvious from the context. > Done. > * * * > > > + if (a->expr->expr_type == EXPR_CONDITIONAL > > + && gfc_is_conditional_nil (a->expr) && !optional_dummy) > > + { > > + gfc_error ( > > + "Dummy argument is not optional, .NIL. at %L shall not appear", > > + &a->expr->where); > > That's a comma splice; I think it reads better as: > > "As the dummy argument is not optional, .NIL. at %L shall not appear" > Done. > * * * > > > - if (e->expr_type != EXPR_VARIABLE) > > + if (e->expr_type != EXPR_VARIABLE && e->expr_type != EXPR_CONDITIONAL) > > I wonder whether we should do here as well: > > if (e->expr_type != EXPR_VARIABLE > && e->expr_type != EXPR_CONDITIONAL /* not .NIL. */) > > to make clear why only EXPR_CONDITIONAL has a special case. > Done. > * * * > > > @@ -12770,6 +12770,9 @@ gfc_walk_conditional_expr (gfc_ss *ss, gfc_expr > > *expr) > > { > > gfc_ss *head; > > > > + if (!expr->value.conditional.condition) > > + return gfc_ss_terminator; > > For now … Obviously, it needs to be revisited when adding array support. > > * * * > > > --- a/gcc/fortran/trans-expr.cc > > +++ b/gcc/fortran/trans-expr.cc > > @@ -4375,6 +4375,12 @@ gfc_conv_conditional_expr (gfc_se *se, gfc_expr > > *expr) > > tree condition, true_val, false_val; > > tree type; > > > > + if (!expr->value.conditional.condition) > > + { > > + se->expr = build_empty_stmt (input_location); > > This requires more for character strings, VALUE+OPTIONAL etc. > as discussed above. > > Can you add '/* Handle .NIL. */ above? It is not obvious > from the code that this is about .NIL. - and can also not > be gathered by looking a few lines above or below. Thus, > the comment will help. > Done. > * * * > > > +program conditional_nil > > + implicit none > > + integer :: a = 4 > > + integer :: b = 6 > > + > > + call five((a < 5 ? a : .NIL.)) > > + if (a /= 5) stop 1 > > + call five((a == 5 ? .NIL. : a)) > > + if (a /= 5) stop 2 > > This test is not ideal: 'five' sets 'a = 5' but you already have 5. > How about using, e.g., > a = 42 > call five((a == 42 ? .NIL. : a)) > if (a /= 42) stop 2 > here? > This is a great test case because it revealed a subtle bug in the patch: when retrieving the optional pointer, we should use null_pointer_node instead of NOP_EXPR from build_empty_statement. Using the latter introduces subtle bugs into the CFG - I suspect some missing GOTOs - which ultimately leads to errors. > * * * > > > + call five((a /= 5 ? .NIL. : b > 5 ? b : .NIL.)) > > + if (b /= 5) stop 3 > > Same issue, exceptionally, here. > Changed accordingly. > * * * > > > +++ b/gcc/testsuite/gfortran.dg/conditional_11.f90 > > […] > > + i = (i > 0 ? 1 : .nil.) ! { dg-error "is only valid in conditional > > arguments" } > > + i = (i > 0 ? .nil. : .nil.) ! { dg-error "Both operands of the > > conditional argument at" } > > + call five((i < 5 ? i : i > 43 ? i : .nil.)) ! { dg-error "Dummy argument > > is not optional, .NIL. at" } > > The second line is wrong for two reasons: (A) at least one > expression must be not .NIL. and (B) .NIL. is only valid in > conditional arguments. > > I think it is better to add a new subroutine - that takes > an optional argument - and then check for (B) only. > > That's again just a minor test nit - as the code works > and effectively also this check. > Fixed. The updated patch reorders the check to first resolve true_expr and false_expr before testing if both branches are nil. If either expression resolves to an unintended nil, checking the other branch becomes unnecessary. > * * * > > Can you also add some test for an actual argument with > conditions that is not a conditional argument, e.g., > > call five ( 1 + (cond ? 1 : .NIL. ) ) > > In that case, .NIL. is invalid - and the code handles this; > however, a testcase is missing for it. > Added in conditional_11.f90. Thanks for the really detailed review! Yuao
0001-fortran-support-.NIL.-in-conditional-arguments.patch
Description: Binary data
