LLD symbol ordering file binary name tag

[shenhan]

Summary

People use --symbol-ordering-file to do optimized linking, and sometimes, this option is passed at config time via "LD_FLAGS=-Wl,--symbol-ordering-file=<filename>", and this applies to every linker command - including dependent builds and small feature-testing builds performed during config time. This causes huge build time increase.

We would like to propose adding an optional "# apply_if_output_is: <my.out>" tag at the front of the file to say that it is meant for binary named "my.out". The way it should work is that if the binary name is different then the ordering is silently ignored.

We believe this is a small but useful feature for various scenarios where people do optimized linking by specify symbol orderings.

(Without the patch, "LD_FLAGS=-Wl,--symbol-ordering-file=<filename>" increased config time for "mysql" from 50 seconds to 10 minutes, and with the patch, the build time drops to 51 seconds.)

[MaskRay]

(Without the patch, "LD_FLAGS=-Wl,--symbol-ordering-file=<filename>" increased config time for "mysql" from 50 seconds to 10 minutes, and with the patch, the build time drops to 51 seconds.)

Can you share a reproduce file somewhere? It is hard to believe that --symbol-ordering-file can be so slow. I think we should consider optimizing it before adding new directives.

[shenhan]

Hi Fangrui, I've put up a reproduce case for mysql here. These are steps:

1. git clone [e-mail]:shenhanc78/mysql-benchmark.git
2. cd mysql-benchmark/
3. Download mysql-server-mysql-8.0.21: a. git clone [e-mail]:mysql/mysql-server.git b. git fetch --all --tags c. git checkout tags/mysql-8.0.21 -b mysql-8.0.21-branch
4. Patch the cmake files using lld_build.patch provided.
5. Install prerequisites: sysbench libssl-dev bison.
6. make sure master-built clang is in $PATH
7. ./do-config.sh and observe the long configuration time Comment out the ${SYM_OPT} and observe the difference

Thanks.

[MaskRay]

In D87121#2256663, @shenhan wrote:

Hi Fangrui, I've put up a reproduce case for mysql here. These are steps:

1. git clone [e-mail]:shenhanc78/mysql-benchmark.git
2. cd mysql-benchmark/
3. Download mysql-server-mysql-8.0.21: a. git clone [e-mail]:mysql/mysql-server.git b. git fetch --all --tags c. git checkout tags/mysql-8.0.21 -b mysql-8.0.21-branch
4. Patch the cmake files using lld_build.patch provided.
5. Install prerequisites: sysbench libssl-dev bison.
6. make sure master-built clang is in $PATH
7. ./do-config.sh and observe the long configuration time Comment out the ${SYM_OPT} and observe the difference

Thanks.

Thanks. I added --debug-trycompile to the cmake command line and I can see that the cmake command runs lld many times. --symbol-ordering-file tmp/p/mysql-benchmark/propeller/propeller.symorder is used for every invocation. propeller.symorder is a large file (8.2MiB) and about 0.05s is spent on reading/processing --symbol-ordering-file (when the option is used local symbols are processed as well - this makes it even slower).

The main slowdown is due to warnings like ld.lld: warning: symbol ordering file: no such symbol: _ZNK30Item_func_internal_data_length8functypeEv. It can take 0.4s (/dev/null) or 1s (/dev/tty). I think the fix is --no-warn-symbol-ordering (@shenhan

-- Found Git: /usr/bin/git  
-- Performing Test check_c_compiler_flag__fdiagnostics_color_always
-- Performing Test check_c_compiler_flag__fdiagnostics_color_always - Success
-- Performing Test check_c_compiler_flag__Wall
-- Performing Test check_c_compiler_flag__Wall - Success
-- Performing Test check_c_compiler_flag__Wextra
-- Performing Test check_c_compiler_flag__Wextra - Success
-- Performing Test check_c_compiler_flag__Wno_unused_parameter
-- Performing Test check_c_compiler_flag__Wno_unused_parameter - Success
-- Performing Test check_c_compiler_flag__Wstrict_aliasing
-- Performing Test check_c_compiler_flag__Wstrict_aliasing - Success
-- Performing Test check_c_compiler_flag__Wstrict_prototypes

[MaskRay]

I also created D87272 to make bulky stderr write faster. I think this patch can be abandoned.

[shenhan]

Hi Maskray, thanks for the fix. Another latent concern is that apply the symbol ordering file unconditionally might cause unintentional consequence to the final binary to the extent that the binary might be corrupted or less optimized, in this sense, this patch might still be warranted (orthogonal to your CL), what do you think?

[MaskRay]

In D87121#2261061, @shenhan wrote:

Hi Maskray, thanks for the fix. Another latent concern is that apply the symbol ordering file unconditionally might cause unintentional consequence to the final binary to the extent that the binary might be corrupted or less optimized, in this sense, this patch might still be warranted (orthogonal to your CL), what do you think?

I think it is important for the user to specify the appropriate linker flags. None of the other flags (even similar ones, e.g. --dynamic-list, -T, --version-script) should have such an exclusion mechanism (one concern is that changing -o can change the behavior, which is not great). There may be someway for the mysql build system to not apply the linker options at config stage, or you should just hook the final link. For your case --no-warn-symbol-ordering can essentially nullify the time increase caused by --symbol-ordering-file.

[shenhan]

In D87121#2261176, @MaskRay wrote:

In D87121#2261061, @shenhan wrote:

Hi Maskray, thanks for the fix. Another latent concern is that apply the symbol ordering file unconditionally might cause unintentional consequence to the final binary to the extent that the binary might be corrupted or less optimized, in this sense, this patch might still be warranted (orthogonal to your CL), what do you think?

I think it is important for the user to specify the appropriate linker flags. None of the other flags (even similar ones, e.g. --dynamic-list, -T, --version-script) should have such an exclusion mechanism (one concern is that changing -o can change the behavior, which is not great). There may be someway for the mysql build system to not apply the linker options at config stage, or you should just hook the final link. For your case --no-warn-symbol-ordering can essentially nullify the time increase caused by --symbol-ordering-file.

Hi Maskray, thanks, I see your point here. We are thinking of providing the users a more straightforward way to pinpoint the ordering file to the designated binary, this is not only for mysql, we have had similar situations for clang, firefox, chromium and various internal applications, and there is no universal way to specify linkopt only to certain binaries (chrome for chromium, libxul.so for firefox, mysqld for mysql-server, etc). What if we use another way to specify the ordering file name tag, say, "--symbol-ordering-file=order.txt[;name_tag=xxx]", this is totally optional, does not alter the interpretation of current symbol order file and is easy to spot if there is any inconsistency between "-o name" and the name_tag attribute (and I believe this would also benefit --dynamic-list or --version-script) What do you think?

[MaskRay]

In D87121#2261282, @shenhan wrote:

In D87121#2261176, @MaskRay wrote:

In D87121#2261061, @shenhan wrote:

Hi Maskray, thanks for the fix. Another latent concern is that apply the symbol ordering file unconditionally might cause unintentional consequence to the final binary to the extent that the binary might be corrupted or less optimized, in this sense, this patch might still be warranted (orthogonal to your CL), what do you think?

I think it is important for the user to specify the appropriate linker flags. None of the other flags (even similar ones, e.g. --dynamic-list, -T, --version-script) should have such an exclusion mechanism (one concern is that changing -o can change the behavior, which is not great). There may be someway for the mysql build system to not apply the linker options at config stage, or you should just hook the final link. For your case --no-warn-symbol-ordering can essentially nullify the time increase caused by --symbol-ordering-file.

Hi Maskray, thanks, I see your point here. We are thinking of providing the users a more straightforward way to pinpoint the ordering file to the designated binary, this is not only for mysql, we have had similar situations for clang, firefox, chromium and various internal applications, and there is no universal way to specify linkopt only to certain binaries (chrome for chromium, libxul.so for firefox, mysqld for mysql-server, etc). What if we use another way to specify the ordering file name tag, say, "--symbol-ordering-file=order.txt[;name_tag=xxx]", this is totally optional, does not alter the interpretation of current symbol order file and is easy to spot if there is any inconsistency between "-o name" and the name_tag attribute (and I believe this would also benefit --dynamic-list or --version-script) What do you think?

Both the # apply_if_output_is directive and the --symbol-ordering-file=order.txt[;name_tag=xxx] approach have the problem that changing -o can affect the program behavior.

This may easily trip up the users. IIUC the Propeller optimization framework (post-link optimization) needs a profiling stage like PGO. Just like that if you don't want PGO/ThinLTO to interfere with regular compiles, you may need some plumbering in the build system.

If I had to accept one approach, the command option approach might look better to me as it is more obvious.
(--dynamic-list and --version-script do not really benefit from the syntax because they are usually part of the existing build system. --symbol-ordering-file is different (including the --warn-symbol-ordering diagnostic option) as it is usually an exotic thing)

I still sympathize with the feeling that --symbol-ordering-file may not be easily integrated (it is generic and not specific to Propeller), so
I am adding some folks for ideas: @grimar @jhenderson @nickdesaulniers @psmith @thakis

[jhenderson]

It seems in general that a build system will want a different symbol ordering file per output, so any involving more than one output would need them specified individually. I don't think a global variable that sets it for all links involved in a build (including CMake links) really makes sense as a result. Limiting it down to a single file via any mechanism (whether within the file or command-line option) sounds like a sticking plaster solution rather than actually the right fix. As @MaskRay has already mentioned, the fix is to specify the right options for the right link. I would think the same applies for options like --dynamic-list, since you might want to export different symbols for different links. Usually the way to do this is with a lower-level customisation of linker options.

At the same time, I can see the benefit of being able to specify the option at a higher level, especially if a project only has one output (which sounds like is the case for mysql?), rather than deep inside some build script. Perhaps CMake needs a separate concept of linker flags used in all builds, including configuration steps, and flags that are specific to specific executables, but which can be configured via the command-line/CMake GUI. Of course, you could achieve the latter easily enough, since you can create your own CMake flags. For example, you could have an "OUTPUT_SPECIFIC_LD_FLAGS" custom option with value "$(Output1)=--symbol-ordering-file=order1.txt;$(Output2)==--symbol-ordering-file=order2.txt" etc (where $(Output1) etc are other CMake variables - I can't remember the syntax for doing that).

Summary: I don't think this is an LLD issue, and I don't think you should be specifying --symbol-ordering-file=<some file> for all outputs. This is a build system issue, and the build system needs configuring in the right way.

[psmith]

Will have to have a think. My initial reaction to "apply this file only when these conditions hold" is akin to moving part of the build system into the linker. It could be done, but I'm not sure it should be. For example if there were a linker script it would have to precisely match the inputs to make sense.

lld/ELF/Driver.cpp

static std::pair<StringRef, StringRef> getOldNewOptions(opt::InputArgList &args,
 
   StringRef s = arg->getValue();
   std::pair<StringRef, StringRef> ret = s.split(';');
   if (ret.second.empty())
     error(getAliasSpelling(arg) + " expects 'old;new' format, but got " + s);
   return ret;
 }
 
+// Return false iff the first line of symbol ordering file is "#
+// apply_if_output_is: <filename>", and `basename <filename>` does not equal to
+// `basename config->outputFile`.
+static bool shouldApplySymbolOrderingFile(MemoryBufferRef mb) {
+  StringRef buf = mb.getBuffer();
+  if (buf.empty() || buf[0] != '#')
+    return true;
+  size_t pos = buf.find_first_of('\n');
+  if (pos == StringRef::npos)
+    return true;
+  // "line" starts from the first non-space character after "#".
+  StringRef line = buf.substr(1, pos).trim();
+  if (line.consume_front("apply_if_output_is:")) {
+    // Remove any possible spaces betwee "apply_if_output_is:" and "<filename>".
+    StringRef filename = line.ltrim();
+    return path::filename(filename) == path::filename(config->outputFile);
+  }
+  return true;
+}
+
 // Parse the symbol ordering file and warn for any duplicate entries.
 static std::vector<StringRef> getSymbolOrderingFile(MemoryBufferRef mb) {
+  if (!shouldApplySymbolOrderingFile(mb))
+    return {};
   SetVector<StringRef> names;
   for (StringRef s : args::getLines(mb))
     if (!names.insert(s) && config->warnSymbolOrdering)
       warn(mb.getBufferIdentifier() + ": duplicate ordered symbol: " + s);
 
   return names.takeVector();
 }
 

lld/ELF/Options.td

 
 def start_lib: F<"start-lib">,
   HelpText<"Start a grouping of objects that should be treated as if they were together in an archive">;
 
 def strip_all: F<"strip-all">, HelpText<"Strip all symbols">;
 
 def strip_debug: F<"strip-debug">, HelpText<"Strip debugging information">;
 
-defm symbol_ordering_file:
-  Eq<"symbol-ordering-file", "Layout sections to place symbols in the order specified by symbol ordering file">;
+defm symbol_ordering_file
+    : Eq<"symbol-ordering-file", "Layout sections to place symbols in the "
+                                 "order specified by symbol ordering file, "
+                                 "an optional \"# apply_if_output_is: "
+                                 "<filename>\" can be specified at the "
+                                 "beginning to mean this file is only to be "
+                                 "used when linking <filename>">;
 
 defm sysroot: Eq<"sysroot", "Set the system root">;
 
 def target1_rel: F<"target1-rel">, HelpText<"Interpret R_ARM_TARGET1 as R_ARM_REL32">;
 
 def target1_abs: F<"target1-abs">, HelpText<"Interpret R_ARM_TARGET1 as R_ARM_ABS32 (default)">;
 
 defm target2:

lld/test/ELF/symbol-ordering-file-apply-if-output-is.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: echo "_foo2" > %t_order.txt
+# RUN: echo "_foo1" >> %t_order.txt
+
+# RUN: ld.lld --symbol-ordering-file %t_order.txt %t.o -o %t2.out
+# RUN: llvm-objdump -s %t2.out| FileCheck %s --check-prefix=ORDER_FILE
+
+# ORDER_FILE:      Contents of section .foo:
+# ORDER_FILE-NEXT:  2211
+
+# RUN: echo "# apply_if_output_is: dummy_file_name_no_match" > %t_order.txt
+# RUN: echo "_foo2" >> %t_order.txt
+# RUN: echo "_foo1" >> %t_order.txt
+
+# RUN: ld.lld --symbol-ordering-file %t_order.txt %t.o -o %t2.out
+# RUN: llvm-objdump -s %t2.out| FileCheck %s --check-prefix=ORDER_FILE_WITH_APPLY
+
+# ORDER_FILE_WITH_APPLY:      Contents of section .foo:
+# ORDER_FILE_WITH_APPLY-NEXT:  1122
+
+# RUN: echo "# apply_if_output_is: %t2.out" > %t_order.txt
+# RUN: echo "_foo2" >> %t_order.txt
+# RUN: echo "_foo1" >> %t_order.txt
+
+# RUN: ld.lld --symbol-ordering-file %t_order.txt %t.o -o %t2.out
+# RUN: llvm-objdump -s %t2.out| FileCheck %s --check-prefix=ORDER_FILE_WITH_APPLY2
+
+# ORDER_FILE_WITH_APPLY2:      Contents of section .foo:
+# ORDER_FILE_WITH_APPLY2-NEXT:  2211
+
+.section .foo,"ax",@progbits,unique,1
+_foo1:
+ .byte 0x11
+
+.section .foo,"ax",@progbits,unique,2
+_foo2:
+ .byte 0x22