[flang] Use internal linkage for string literals

[DavidTruby]

Summary

On Windows, global string literals with "linkonce" linkage is not
supported without using comdat. As a simpler fix than adding comdat
support we can use internal linkage instead.

This fixes a bug where two string literals with the same value in
different fortran files would cause a linker error due to the use
of linkonce linkage.

[jeanPerier]

As a simpler fix than adding comdat support ...

Do you know at which stages we would need to add comdat exactly (is this something that is lacking in MLIR LLVM dialect only)?

This fixes a bug where two string literals with the same value in different fortran files would cause a linker error due to the use of linkonce linkage.

Was this bug happening outside of windows too?

My overall thought: we do not need the linkonce for correctness purpose here, and I am not sure how much binary size this actually allow saving (the only case I can think of is the one where a module would define a huge PARAMETER string that would be used in many different compilation units), so, I would be OK to drop it for strings.

But there is another case where we use linkonce_odr and actually need it for correctness purposes: derived type descriptor for types defined in modules.

The runtime requires that two instance of the same derived type in the same program have the same derived type descriptor address. We cannot emit derived type descriptors with external in the module that is defining them because that will not be possible with parametrized derived types, where the instance may be done in different compilation units (derived type descriptors depends on kind parameter values). So I do not think there is a way around comdat here.

https://github.com/llvm/llvm-project/blob/8df5913250d55883feb8fa46a838c93a77c2e291/flang/lib/Lower/ConvertVariable.cpp#L529
https://github.com/llvm/llvm-project/blob/9470169fcbe93ccf9f97b56bfb23c636e32133d8/flang/runtime/derived-api.cpp#L104

[DavidTruby]

Essentially, at an LLVM IR level, for each global variable with linkonce linkage you need a comdat declaration for linkage to work on Windows. For example:

$foo = comdat any
@foo = linkonce constant [5 x i8] c"hello"

MLIR's LLVM dialect doesn't seem to support the comdat construct here so we would need to add support there. I think this is only needed for Windows but from what I can tell it's just ignored on other platforms so we could just add it every time we have a linkonce global.

I was concerned when posting this patch that we do use linkonce elsewhere and will need to consider this for those usages too, but this fixes a specific bug we are seeing. Once we have comdat support I think using linkonce for strings is also fine, so we could revert this.

I can look in to how difficult adding the comdat construct would be

[DavidTruby]

I'm currently working on adding support for comdat to MLIR. Would you be ok with merging this to fix the string bug while I am working on that?

[jeanPerier]

In D149859#4332240, @DavidTruby wrote:

I'm currently working on adding support for comdat to MLIR. Would you be ok with merging this to fix the string bug while I am working on that?

Thanks for working on comdat!
And yes, you can merge this, but please add a TODO comment near createInternalLinkage that we will want to go back to linkonce/linkonce_odr when comdat is supported.

flang/lib/Optimizer/Builder/FIRBuilder.cpp

fir::ExtendedValue fir::factory::createStringLiteral(fir::FirOpBuilder &builder,
   auto global = builder.getNamedGlobal(globalName);
   if (!global)
     global = builder.createGlobalConstant(
         loc, type, globalName,
         [&](fir::FirOpBuilder &builder) {
           auto stringLitOp = builder.createStringLitOp(loc, str);
           builder.create<fir::HasValueOp>(loc, stringLitOp);
         },
-        builder.createLinkOnceLinkage());
+        builder.createInternalLinkage());
+  // TODO: This can be changed to linkonce linkage once we have support for
+  // generating comdat sections
   auto addr = builder.create<fir::AddrOfOp>(loc, global.resultType(),
                                             global.getSymbol());
   auto len = builder.createIntegerConstant(
       loc, builder.getCharacterLengthType(), str.size());
   return fir::CharBoxValue{addr, len};
 }
 
 llvm::SmallVector<mlir::Value>

flang/test/Lower/allocatable-assignment.f90

 ! CHECK:           %[[VAL_62:.*]] = fir.shape %[[VAL_3]]#1 : (index) -> !fir.shape<1>
 ! CHECK:           %[[VAL_63:.*]] = fir.embox %[[VAL_61]]#1(%[[VAL_62]]) : (!fir.heap<!fir.array<?xf32>>, !fir.shape<1>) -> !fir.box<!fir.heap<!fir.array<?xf32>>>
 ! CHECK:           fir.store %[[VAL_63]] to %[[VAL_0]] : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf32>>>>
 ! CHECK:         }
 ! CHECK:         return
 ! CHECK:       }
 end subroutine
 
-! CHECK: fir.global linkonce @[[error_message]] constant : !fir.char<1,76> {
+! CHECK: fir.global internal @[[error_message]] constant : !fir.char<1,76> {
 ! CHECK:   %[[msg:.*]] = fir.string_lit "array left hand side must be allocated when the right hand side is a scalar\00"(76) : !fir.char<1,76>
 ! CHECK:   fir.has_value %[[msg:.*]] : !fir.char<1,76>
 ! CHECK: }
 
 end module
 
 !  use alloc_assign
 !  real :: y(2, 3) = reshape([1,2,3,4,5,6], [2,3])
 !  real, allocatable :: x (:, :)
 !  allocate(x(2,2))
 !  call test_with_lbounds(x, y) 
 !  print *, x(10, 20)
 !  print *, x
 !end

flang/test/Lower/character-assignment.f90

subroutine assign_zero_size_array(n)
     ! CHECK:       %{{.*}} = fir.coordinate_of %{{.*}}, %[[ARG_0]] : (!fir.ref<!fir.array<?x!fir.char<1>>>, index) -> !fir.ref<!fir.char<1>>
     ! CHECK:   fir.if %[[VAL_2]]#0 {
     ! CHECK:     fir.if %[[VAL_1]] {
     ! CHECK:     fir.store %[[VAL_2]]#1 to %[[VAL_0]] : !fir.ref<!fir.heap<!fir.array<?x!fir.char<1,?>>>>
     a = [character(n)::]
     ! CHECK:   return
   end subroutine
 
-! CHECK-LABEL: fir.global linkonce @_QQcl.48656C6C6F20576F726C64
+! CHECK-LABEL: fir.global internal @_QQcl.48656C6C6F20576F726C64
 ! CHECK: %[[lit:.*]] = fir.string_lit "Hello World"(11) : !fir.char<1,11>
 ! CHECK: fir.has_value %[[lit]] : !fir.char<1,11>
 ! CHECK: }

flang/test/Lower/convert.f90

 ! ALL: %[[VAL_1:.*]] = fir.address_of(@[[FC_STR:.*]]) : !fir.ref<!fir.char<1,13>>
 ! ALL: %[[VAL_3:.*]] = fir.convert %[[VAL_1]] : (!fir.ref<!fir.char<1,13>>) -> !fir.ref<i8>
 ! ALL: %[[VAL_4:.*]] = fir.insert_value %[[VAL_0]], %[[VAL_3]], [0 : index, 0 : index] : (!fir.array<1xtuple<!fir.ref<i8>, !fir.ref<i8>>>, !fir.ref<i8>) -> !fir.array<1xtuple<!fir.ref<i8>, !fir.ref<i8>>>
 ! ALL: %[[VAL_5:.*]] = fir.address_of(@[[OPT_STR:.*]]) : !fir.ref<!fir.char<1,[[OPT_STR_LEN:.*]]>>
 ! ALL: %[[VAL_7:.*]] = fir.convert %[[VAL_5]] : (!fir.ref<!fir.char<1,[[OPT_STR_LEN]]>>) -> !fir.ref<i8>
 ! ALL: %[[VAL_8:.*]] = fir.insert_value %[[VAL_4]], %[[VAL_7]], [0 : index, 1 : index] : (!fir.array<1xtuple<!fir.ref<i8>, !fir.ref<i8>>>, !fir.ref<i8>) -> !fir.array<1xtuple<!fir.ref<i8>, !fir.ref<i8>>>
 ! ALL: fir.has_value %[[VAL_8]] : !fir.array<1xtuple<!fir.ref<i8>, !fir.ref<i8>>>
 
-! ALL: fir.global linkonce @[[FC_STR]] constant : !fir.char<1,13> {
+! ALL: fir.global internal @[[FC_STR]] constant : !fir.char<1,13> {
 ! ALL: %[[VAL_0:.*]] = fir.string_lit "FORT_CONVERT\00"(13) : !fir.char<1,13>
 ! ALL: fir.has_value %[[VAL_0]] : !fir.char<1,13>
 
-! ALL: fir.global linkonce @[[OPT_STR]] constant : !fir.char<1,[[OPT_STR_LEN]]> {
+! ALL: fir.global internal @[[OPT_STR]] constant : !fir.char<1,[[OPT_STR_LEN]]> {
 ! UNKNOWN: %[[VAL_0:.*]] = fir.string_lit "UNKNOWN\00"([[OPT_STR_LEN]]) : !fir.char<1,[[OPT_STR_LEN]]>
 ! NATIVE: %[[VAL_0:.*]] = fir.string_lit "NATIVE\00"([[OPT_STR_LEN]]) : !fir.char<1,[[OPT_STR_LEN]]>
 ! LITTLE_ENDIAN: %[[VAL_0:.*]] = fir.string_lit "LITTLE_ENDIAN\00"([[OPT_STR_LEN]]) : !fir.char<1,[[OPT_STR_LEN]]>
 ! BIG_ENDIAN: %[[VAL_0:.*]] = fir.string_lit "BIG_ENDIAN\00"([[OPT_STR_LEN]]) : !fir.char<1,[[OPT_STR_LEN]]>
 ! SWAP: %[[VAL_0:.*]] = fir.string_lit "SWAP\00"([[OPT_STR_LEN]]) : !fir.char<1,[[OPT_STR_LEN]]>
 ! ALL: fir.has_value %[[VAL_0]] : !fir.char<1,[[OPT_STR_LEN]]>
 
 ! ALL: fir.global linkonce @_QQEnvironmentDefaults.list constant : tuple<i[[int_size:.*]], !fir.ref<!fir.array<1xtuple<!fir.ref<i8>, !fir.ref<i8>>>>> {

flang/test/Lower/global-format-strings.f90

 ! RUN: bbc -emit-fir -o - %s | FileCheck %s
 
 ! Test checks whether the text of the format statement is hashconed into a
 ! global similar to a CHARACTER literal and then referenced.
 
 program other
   write(10, 1008)
   ! CHECK:  fir.address_of(@{{.*}}) :
 1008 format('ok')
 end
-! CHECK-LABEL: fir.global linkonce @_QQcl.28276F6B2729 constant
+! CHECK-LABEL: fir.global internal @_QQcl.28276F6B2729 constant
 ! CHECK: %[[lit:.*]] = fir.string_lit "('ok')"(6) : !fir.char<1,6>
 ! CHECK: fir.has_value %[[lit]] : !fir.char<1,6>
 ! CHECK: }

flang/test/Lower/io-statement-open-options.f90

subroutine test_convert_specifier(unit)
   ! CHECK: %[[be_str_conv:.*]] = fir.convert %[[be_str]] : (!fir.ref<!fir.char<1,10>>) -> !fir.ref<i8>
   ! CHECK: %[[len_conv:.*]] = fir.convert %[[len]] : (index) -> i64
   ! CHECK: %{{.*}} = fir.call @_FortranAioSetConvert(%[[cookie]], %[[be_str_conv]], %[[len_conv]]) {{.*}}: (!fir.ref<i8>, !fir.ref<i8>, i64) -> i1
   ! CHECK: %{{.*}} = fir.call @_FortranAioEndIoStatement(%[[cookie]]) {{.*}}: (!fir.ref<i8>) -> i32
   open(unit, form="UNFORMATTED", convert="BIG_ENDIAN")
   close(unit)
 end subroutine
 
-! CHECK: fir.global linkonce @[[be_str_name]] constant : !fir.char<1,10> {
+! CHECK: fir.global internal @[[be_str_name]] constant : !fir.char<1,10> {
 ! CHECK: %[[be_str_lit:.*]] = fir.string_lit "BIG_ENDIAN"(10) : !fir.char<1,10>
 ! CHECK: fir.has_value %[[be_str_lit]] : !fir.char<1,10>

flang/test/Lower/namelist.f90

subroutine global_pointer
   ! CHECK: %[[a2:.*]] = fir.call @_FortranAioBeginExternalListOutput({{.*}}, %[[a1]], {{.*}}) {{.*}}: (i32, !fir.ref<i8>, i32) -> !fir.ref<i8>
   ! CHECK: %[[a3:.*]] = fir.address_of
   ! CHECK: %[[a4:.*]] = fir.convert %[[a3]]
   ! CHECK: %[[a5:.*]] = fir.call @_FortranAioOutputNamelist(%[[a2]], %[[a4]])
   ! CHECK: %[[a6:.*]] = fir.call @_FortranAioEndIoStatement(%[[a2]])
   write(10, nml=mygroup)
 end
 
-  ! CHECK-DAG: fir.global linkonce @_QQcl.6A6A6A00 constant : !fir.char<1,4>
-  ! CHECK-DAG: fir.global linkonce @_QQcl.63636300 constant : !fir.char<1,4>
-  ! CHECK-DAG: fir.global linkonce @_QQcl.6E6E6E00 constant : !fir.char<1,4>
+  ! CHECK-DAG: fir.global internal @_QQcl.6A6A6A00 constant : !fir.char<1,4>
+  ! CHECK-DAG: fir.global internal @_QQcl.63636300 constant : !fir.char<1,4>
+  ! CHECK-DAG: fir.global internal @_QQcl.6E6E6E00 constant : !fir.char<1,4>

flang/test/Lower/read-write-buffer.f90

 subroutine some()
   character(LEN=255):: buffer
   character(LEN=255):: greeting
 10 format (A255)
   ! CHECK:  fir.address_of(@_QQcl.636F6D70696C6572) :
   write (buffer, 10) "compiler"
   read (buffer, 10) greeting
 end
-! CHECK-LABEL: fir.global linkonce @_QQcl.636F6D70696C6572
+! CHECK-LABEL: fir.global internal @_QQcl.636F6D70696C6572
 ! CHECK: %[[lit:.*]] = fir.string_lit "compiler"(8) : !fir.char<1,8>
 ! CHECK: fir.has_value %[[lit]] : !fir.char<1,8>
 ! CHECK: }

flang/unittests/Optimizer/Builder/FIRBuilderTest.cpp

TEST_F(FIRBuilderTest, createStringLiteral) {
   auto generalGetLen = fir::getLen(strLit);
   checkIntegerConstant(generalGetLen, builder.getCharacterLengthType(), 16);
   auto addr = charBox->getBuffer();
   EXPECT_TRUE(mlir::isa<fir::AddrOfOp>(addr.getDefiningOp()));
   auto addrOp = dyn_cast<fir::AddrOfOp>(addr.getDefiningOp());
   auto symbol = addrOp.getSymbol().getRootReference().getValue();
   auto global = builder.getNamedGlobal(symbol);
   EXPECT_EQ(
-      builder.createLinkOnceLinkage().getValue(), global.getLinkName().value());
+      builder.createInternalLinkage().getValue(), global.getLinkName().value());
   EXPECT_EQ(fir::CharacterType::get(builder.getContext(), 1, strValue.size()),
       global.getType());
 
   auto stringLitOps = global.getRegion().front().getOps<fir::StringLitOp>();
   EXPECT_TRUE(llvm::hasSingleElement(stringLitOps));
   for (auto stringLit : stringLitOps) {
     EXPECT_EQ(16, stringLit.getSize().cast<mlir::IntegerAttr>().getValue());
     EXPECT_TRUE(stringLit.getValue().isa<StringAttr>());