[PATCH] D122450: [ELF] Default to --no-fortran-common

[Fangrui Song via Phabricator via llvm-commits]

MaskRay created this revision.
MaskRay added reviewers: bd1976llvm, ikudrin, peter.smith, sfertile.
Herald added subscribers: StephenFan, arichardson, emaste.
Herald added a reviewer: ctetreau.
Herald added a project: All.
MaskRay requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

D86142 <https://reviews.llvm.org/D86142> introduced --fortran-common and defaulted it to true (matching GNU ld
but deviates from gold/macOS ld64). The default state was motivated by transparently
supporting some FORTRAN 77 programs (Fortran 90 deprecated common blocks).
Now I think it again. I believe we made a mistake to change the default:

- this is a weird and legacy rule, though the breakage is very small
- --fortran-common introduced complexity to parallel symbol resolution and will slow down it
- --fortran-common more likely causes issues when users mix COMMON and STB_GLOBAL definitions (see https://github.com/llvm/llvm-project/issues/48570 and https://maskray.me/blog/2022-02-06-all-about-common-symbols). I have seen several issues in our internal projects and Android. On the other hand, --no-fortran-common is safer since COMMON/STB_GLOBAL have the same semantics related to archive member extraction.

Therefore I think we should switch back, not punishing the common uage.
A platform wanting --fortran-common can implement ld.lld as a shell script
wrapper around `lld -flavor gnu --fortran-common "$@"`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122450

Files:
  lld/ELF/Driver.cpp
  lld/docs/ReleaseNotes.rst
  lld/test/ELF/common-archive-lookup.s
  lld/test/ELF/warn-backrefs.s


Index: lld/test/ELF/warn-backrefs.s
===================================================================
--- lld/test/ELF/warn-backrefs.s
+++ lld/test/ELF/warn-backrefs.s
@@ -77,7 +77,7 @@
 # RUN: llvm-ar rcs %tcomm.a %tcomm.o
 # RUN: llvm-ar rcs %tstrong.a %tstrong.o
 # RUN: ld.lld --warn-backrefs %tcomm.a %t1.o %t5.o 2>&1 -o /dev/null | FileCheck --check-prefix=COMM %s
-# RUN: ld.lld --fatal-warnings --warn-backrefs %tcomm.a %t1.o %t5.o %tstrong.a 2>&1 -o /dev/null
+# RUN: ld.lld --fatal-warnings --fortran-common --warn-backrefs %tcomm.a %t1.o %t5.o %tstrong.a 2>&1 -o /dev/null
 # RUN: ld.lld --warn-backrefs --no-fortran-common %tcomm.a %t1.o %t5.o %tstrong.a 2>&1 -o /dev/null | FileCheck --check-prefix=COMM %s
 
 # COMM: ld.lld: warning: backward reference detected: obj in {{.*}}5.o refers to {{.*}}comm.a
Index: lld/test/ELF/common-archive-lookup.s
===================================================================
--- lld/test/ELF/common-archive-lookup.s
+++ lld/test/ELF/common-archive-lookup.s
@@ -25,10 +25,10 @@
 ## Bitcode archive.
 # RUN: llvm-ar crs 4.a 1.bc 2.bc
 
-# RUN: ld.lld -o 1 main.o 1.a
+# RUN: ld.lld -o 1 main.o 1.a --fortran-common
 # RUN: llvm-objdump -D -j .data 1 | FileCheck --check-prefix=TEST1 %s
 
-# RUN: ld.lld -o 2 main.o --start-lib 1.o strong_data_only.o --end-lib
+# RUN: ld.lld -o 2 main.o --start-lib 1.o strong_data_only.o --end-lib --fortran-common
 # RUN: llvm-objdump -D -j .data 2 | FileCheck --check-prefix=TEST1 %s
 
 # RUN: ld.lld -o 3 main.o 2.a
@@ -45,23 +45,25 @@
 # RUN: ld.lld -o 7 main.o 2.o --start-lib 1.o strong_data_only.o --end-lib
 # RUN: llvm-objdump -D -j .data 7 | FileCheck --check-prefix=TEST2 %s
 
-# RUN: not ld.lld -o 8 main.o 1.a strong_data_only.o 2>&1 | \
+# RUN: not ld.lld -o 8 main.o 1.a strong_data_only.o --fortran-common 2>&1 | \
 # RUN:   FileCheck --check-prefix=ERR %s
 
-# RUN: not ld.lld -o 9 main.o --start-lib 1.o 2.o --end-lib  strong_data_only.o 2>&1 | \
+# RUN: not ld.lld -o 9 main.o --start-lib 1.o 2.o --end-lib  strong_data_only.o --fortran-common 2>&1 | \
 # RUN:   FileCheck --check-prefix=ERR %s
 
 # ERR: ld.lld: error: duplicate symbol: block
 
 # RUN: ld.lld --no-fortran-common -o 10 main.o 1.a
 # RUN: llvm-readobj --syms 10 | FileCheck --check-prefix=NFC %s
+# RUN: ld.lld -o 10 main.o 1.a
+# RUN: llvm-readobj --syms 10 | FileCheck --check-prefix=NFC %s
 
 # RUN: ld.lld --no-fortran-common -o 11 main.o --start-lib 1.o strong_data_only.o --end-lib
 # RUN: llvm-readobj --syms 11 | FileCheck --check-prefix=NFC %s
 
-# RUN: ld.lld -o - main.o 4.a --lto-emit-asm | FileCheck --check-prefix=ASM %s
+# RUN: ld.lld -o - main.o 4.a --fortran-common --lto-emit-asm | FileCheck --check-prefix=ASM %s
 
-# RUN: ld.lld -o - main.o  --start-lib 1.bc 2.bc --end-lib --lto-emit-asm | \
+# RUN: ld.lld -o - main.o  --start-lib 1.bc 2.bc --end-lib --fortran-common --lto-emit-asm | \
 # RUN:   FileCheck --check-prefix=ASM %s
 
 ## COMMON overrides weak. Don't extract 3.bc which provides a weak definition.
Index: lld/docs/ReleaseNotes.rst
===================================================================
--- lld/docs/ReleaseNotes.rst
+++ lld/docs/ReleaseNotes.rst
@@ -28,6 +28,7 @@
 
 * ``-z pack-relative-relocs`` is now available to support ``DT_RELR`` for glibc 2.36+.
   (`D120701 <https://reviews.llvm.org/D120701>`_)
+* ``--no-fortran-common`` (pre 12.0.0 behavior) is now the default.
 
 Breaking changes
 ----------------
Index: lld/ELF/Driver.cpp
===================================================================
--- lld/ELF/Driver.cpp
+++ lld/ELF/Driver.cpp
@@ -1058,7 +1058,7 @@
   config->fixCortexA8 =
       args.hasArg(OPT_fix_cortex_a8) && !args.hasArg(OPT_relocatable);
   config->fortranCommon =
-      args.hasFlag(OPT_fortran_common, OPT_no_fortran_common, true);
+      args.hasFlag(OPT_fortran_common, OPT_no_fortran_common, false);
   config->gcSections = args.hasFlag(OPT_gc_sections, OPT_no_gc_sections, false);
   config->gnuUnique = args.hasFlag(OPT_gnu_unique, OPT_no_gnu_unique, true);
   config->gdbIndex = args.hasFlag(OPT_gdb_index, OPT_no_gdb_index, false);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D122450.418111.patch
Type: text/x-patch
Size: 4097 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220325/b12d46da/attachment.bin>