Re: [PATCH v2] Add -fuse-ld= to specify an arbitrary executable as the linker

2020-05-21 Thread Martin Liška

On 5/21/20 1:52 AM, Fangrui Song wrote:

The above issues motivated me to touch this line in PATCH v2.
Dropped in PATCH v2.


Thank you for the updated patch.
The patch is fine except coding style issues:

$ ./contrib/check_GNU_style.py 
/tmp/0001-Add-fuse-ld-to-specify-an-arbitrary-executable-as-th.patch
=== ERROR type #1: blocks of 8 spaces should be replaced with tabs (4 error(s)) 
===
gcc/collect2.c:1135:0:ld_file_name = find_a_file(, 
ld_suffixes[selected_linker], X_OK);
gcc/collect2.c:1137:0: for `ld' (if native linking) or `TARGET-ld' (if 
cross).  */
gcc/collect2.c:1139:0:ld_file_name =
gcc/collect2.c:1140:0:find_a_file(, 
full_ld_suffixes[selected_linker], X_OK);

=== ERROR type #2: there should be exactly one space between function name and 
parenthesis (2 error(s)) ===
gcc/collect2.c:1135:34:ld_file_name = find_a_file(, 
ld_suffixes[selected_linker], X_OK);
gcc/collect2.c:1140:23:find_a_file(, 
full_ld_suffixes[selected_linker], X_OK);

=== ERROR type #3: trailing operator (1 error(s)) ===
gcc/collect2.c:1139:21:ld_file_name =

Note that I can't approve the patch, please wait for a reviewer that can 
approve it.

Martin


[PATCH v2] Add -fuse-ld= to specify an arbitrary executable as the linker

2020-05-20 Thread Fangrui Song via Gcc-patches

On 2020-05-20, Martin Liška wrote:

Hello.


Thanks for review. Sent PATCH v2.


diff --git a/gcc/collect2.c b/gcc/collect2.c
index f8a5ce45994..e04892cb91f 100644
--- a/gcc/collect2.c
+++ b/gcc/collect2.c
@@ -782,15 +782,16 @@ main (int argc, char **argv)
  USE_GOLD_LD,
  USE_BFD_LD,
  USE_LLD_LD,
+  USE_LD,


I wouldn't add a new constant.


  USE_LD_MAX
} selected_linker = USE_DEFAULT_LD;
-  static const char *const ld_suffixes[USE_LD_MAX] =
+  static const char *const ld_suffixes[USE_LD] =
{
  "ld",
  PLUGIN_LD_SUFFIX,
  "ld.gold",
  "ld.bfd",
-  "ld.lld"
+  "ld.lld",


Not needed change.


};
  static const char *const real_ld_suffix = "real-ld";
  static const char *const collect_ld_suffix = "collect-ld";
@@ -802,7 +803,7 @@ main (int argc, char **argv)
  static const char *const strip_suffix = "strip";
  static const char *const gstrip_suffix = "gstrip";
-  const char *full_ld_suffixes[USE_LD_MAX];
+  const char *full_ld_suffixes[USE_LD];
#ifdef CROSS_DIRECTORY_STRUCTURE
  /* If we look for a program in the compiler directories, we just use
 the short name, since these directories are already system-specific.
@@ -844,6 +845,7 @@ main (int argc, char **argv)
  const char **ld1;
  bool use_plugin = false;
  bool use_collect_ld = false;
+  const char *use_ld = NULL;
  /* The kinds of symbols we will have to consider when scanning the
 outcome of a first pass link.  This is ALL to start with, then might
@@ -868,7 +870,7 @@ main (int argc, char **argv)
#endif
  int i;
-  for (i = 0; i < USE_LD_MAX; i++)
+  for (i = 0; i < USE_LD; i++)
full_ld_suffixes[i]
#ifdef CROSS_DIRECTORY_STRUCTURE
  = concat (target_machine, "-", ld_suffixes[i], NULL);
@@ -967,6 +969,11 @@ main (int argc, char **argv)
  selected_linker = USE_GOLD_LD;
else if (strcmp (argv[i], "-fuse-ld=lld") == 0)
  selected_linker = USE_LLD_LD;
+   else if (!strncmp (argv[i], "-fuse-ld=", 9))
+ {
+   use_ld = argv[i] + 9;
+   selected_linker = USE_LD;


Here you can selected_linker = USE_LD_MAX.


+ }
else if (strncmp (argv[i], "-o", 2) == 0)
  {
/* Parse the output filename if it's given so that we can make
@@ -1119,12 +1126,16 @@ main (int argc, char **argv)
}
  /* Search the compiler directories for `ld'.  We have protection against
 recursive calls in find_a_file.  */
-  if (ld_file_name == 0)
-ld_file_name = find_a_file (, ld_suffixes[selected_linker], X_OK);
-  /* Search the ordinary system bin directories
- for `ld' (if native linking) or `TARGET-ld' (if cross).  */
-  if (ld_file_name == 0)
-ld_file_name = find_a_file (, full_ld_suffixes[selected_linker], 
X_OK);


Here you can check either for selected_linker == USE_LD_MAX or for use_ld != 
NULL.


v2 changes to use `path` (PATH) instead. It probably makes more sense.
-fuse-ld=ld.lld -> find a program named ld.lld in PATH.
find_a_file with cpath can unlikely return a result.

Additionally, full_ld_suffixes[selected_linker] does not make sense for
a non-BFD-non-gold linker. (So the existing behavior regarding
-fuse-ld=lld may be strange. I don't intend to change the behavior, though).


+  if (selected_linker == USE_LD) {
+ld_file_name = find_a_file (, use_ld, X_OK);
+  } else {


Bad coding style (braces should be at a separate line) and you don't need the 
{} here.


+if (ld_file_name == 0)
+  ld_file_name = find_a_file (, ld_suffixes[selected_linker], X_OK);
+/* Search the ordinary system bin directories
+   for `ld' (if native linking) or `TARGET-ld' (if cross).  */
+if (ld_file_name == 0)
+  ld_file_name = find_a_file (, full_ld_suffixes[selected_linker], 
X_OK);
+  }
#ifdef REAL_NM_FILE_NAME
  nm_file_name = find_a_file (, REAL_NM_FILE_NAME, X_OK);
@@ -1296,8 +1307,7 @@ main (int argc, char **argv)
 "configuration");
#endif
}
- else if (!use_collect_ld
-  && strncmp (arg, "-fuse-ld=", 9) == 0)
+ else if (strncmp (arg, "-fuse-ld=", 9) == 0)


Why did you change this logic here? Note that it also affects e.g. 
-fuse-ld=gold here.

Martin


Thanks. I noted that the program Debug/gcc/collect-ld is not updated for
-fuse-ld=lld. configure.ac has a problem that if my ld in PATH is
actually /usr/local/bin/ld -> lld, 


ORIGINAL_LD_BFD_FOR_TARGET
and
ORIGINAL_LD_GOLD_FOR_TARGET

can point to non-existent /usr/local/bin/ld.{bfd,gold}

The above issues motivated me to touch this line in PATCH v2.
Dropped in PATCH v2.


{
  /* Do not pass -fuse-ld={bfd|gold|lld} to the linker. */
  ld1--;
diff --git a/gcc/common.opt b/gcc/common.opt
index 4464049fc1f..6408d042d8c 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -2908,6 +2908,10 @@ fuse-ld=lld
Common Driver Negative(fuse-ld=lld)
Use the lld LLVM linker instead of the default linker.
+fuse-ld=
+Common Driver