[PATCH Gold] Recognize clang-style crtbegin and crtend files

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH Gold] Recognize clang-style crtbegin and crtend files

augustine.sterling@gmail.com
For installations of its native implementations of crtbegin and
crtend, Clang uses custom names of the form:

clang_rt.crtbegin[-target-name].o

This patch modifies Layout::match_file_name so that gold treats these
files the same as any other crtbegin and crtend.

2019-11-13  Sterling Augustine <[hidden email]>
       * layout.cc (Layout::match_file_name): Handle clang-style
       crtbegin and crtend filenames.

OK for trunk?

clang_crt.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH Gold] Recognize clang-style crtbegin and crtend files

augustine.sterling@gmail.com
On Wed, Nov 13, 2019 at 2:07 PM [hidden email]
<[hidden email]> wrote:
> 2019-11-13  Sterling Augustine <[hidden email]>
>        * layout.cc (Layout::match_file_name): Handle clang-style
>        crtbegin and crtend filenames.

Ping?
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH Gold] Recognize clang-style crtbegin and crtend files

Fangrui Song-2
On 2019-11-15, [hidden email] wrote:
>On Wed, Nov 13, 2019@2:07 PM [hidden email]
><[hidden email]> wrote:
>> 2019-11-13  Sterling Augustine <[hidden email]>
>>        * layout.cc (Layout::match_file_name): Handle clang-style
>>        crtbegin and crtend filenames.
>
>Ping?

https://sourceware.org/ml/binutils/2019-09/msg00003.html


BTW, the patch looks good to me.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH Gold] Recognize clang-style crtbegin and crtend files

Cary Coutant-3
In reply to this post by augustine.sterling@gmail.com
> This patch modifies Layout::match_file_name so that gold treats these
> files the same as any other crtbegin and crtend.
>
> 2019-11-13  Sterling Augustine <[hidden email]>
>        * layout.cc (Layout::match_file_name): Handle clang-style
>        crtbegin and crtend filenames.

-  if (base_len != match_len + 2 && base_len != match_len + 3)
-    return false;

By removing this, you're allowing the "-<target-name>" part on
gcc-style filenames as well as clang-style. Unless you need to match
that part even without the "clang_rt." prefix, I'd prefer to keep the
stricter matching for gcc-style filenames. And it might be a good idea
to at least check for the "-" before the target name. We just want to
minimize the odds of accidentally matching a user-supplied object
file.

(I really wish there were a better way to handle these sections. I
hope you need this only to support section sorting, and not for the
special treatment of .ctors and .dtors sections, which is a legacy you
shouldn't be saddled with in clang.)

-cary
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH Gold] Recognize clang-style crtbegin and crtend files

Fangrui Song-2
On 2019-12-27, Cary Coutant wrote:

>> This patch modifies Layout::match_file_name so that gold treats these
>> files the same as any other crtbegin and crtend.
>>
>> 2019-11-13  Sterling Augustine <[hidden email]>
>>        * layout.cc (Layout::match_file_name): Handle clang-style
>>        crtbegin and crtend filenames.
>
>-  if (base_len != match_len + 2 && base_len != match_len + 3)
>-    return false;
>
>By removing this, you're allowing the "-<target-name>" part on
>gcc-style filenames as well as clang-style. Unless you need to match
>that part even without the "clang_rt." prefix, I'd prefer to keep the
>stricter matching for gcc-style filenames. And it might be a good idea
>to@least check for the "-" before the target name. We just want to
>minimize the odds of accidentally matching a user-supplied object
>file.
>
>(I really wish there were a better way to handle these sections. I
>hope you need this only to support section sorting, and not for the
>special treatment of .ctors and .dtors sections, which is a legacy you
>shouldn't be saddled with in clang.)
>
>-cary

We can use a looser rule which matches GNU ld more closely.

% ld.bfd --verbose | grep -A12 ' .ctors '
   .ctors          :
   {
     /* gcc uses crtbegin.o to find the start of
        the constructors, so we make sure it is
        first.  Because this is a wildcard, it
        doesn't matter if the user does not
        actually link against crtbegin.o; the
        linker won't look for a file to match a
        wildcard.  The wildcard also means that it
        doesn't matter which directory crtbegin.o
        is in.  */
     KEEP (*crtbegin.o(.ctors))
     KEEP (*crtbegin?.o(.ctors))

If divergence from other compiler-rt files (e.g.
libclang_rt.asan-i386.a) is not a concern, clang_rt.i386.crtbegin.o
should just work with GNU ld.


(On the clang side, I've done https://reviews.llvm.org/D71393 . Since
clang 10, all Linux builds will default to .init_array .
-fno-use-init-array (not recognized by GCC) is required to produce
.ctors)
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH Gold] Recognize clang-style crtbegin and crtend files

augustine.sterling@gmail.com
In reply to this post by Cary Coutant-3
On Fri, Dec 27, 2019 at 4:50 PM Cary Coutant <[hidden email]> wrote:
> By removing this, you're allowing the "-<target-name>" part on
> gcc-style filenames as well as clang-style. Unless you need to match
> that part even without the "clang_rt." prefix, I'd prefer to keep the
> stricter matching for gcc-style filenames. And it might be a good idea
> to at least check for the "-" before the target name. We just want to
> minimize the odds of accidentally matching a user-supplied object
> file.

Thanks for the review.

Enclosed is a patch that handles clang-style names completely
separately, and also
checks for the "-" before the target name.

> (I really wish there were a better way to handle these sections.
+1

> I
> hope you need this only to support section sorting, and not for the
> special treatment of .ctors and .dtors sections, which is a legacy you
> shouldn't be saddled with in clang.)

Unfortunately, we are saddled with this for certain legacy situations,
even with clang. I
wish it weren't so as well.

gold.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH Gold] Recognize clang-style crtbegin and crtend files

Cary Coutant-3
> Enclosed is a patch that handles clang-style names completely
> separately, and also
> checks for the "-" before the target name.

+    if (*(base_name + match_len) != '.' &&
+        *(base_name + match_len) != '-')
+      return false;

Please write these as base_name[match_len].

But Fangrui Song's most recent message in this thread raised a red flag:

> We can use a looser rule which matches GNU ld more closely.
>
> % ld.bfd --verbose | grep -A12 ' .ctors '
>    .ctors          :
>    {
>      /* gcc uses crtbegin.o to find the start of
>         the constructors, so we make sure it is
>         first.  Because this is a wildcard, it
>         doesn't matter if the user does not
>         actually link against crtbegin.o; the
>         linker won't look for a file to match a
>         wildcard.  The wildcard also means that it
>         doesn't matter which directory crtbegin.o
>         is in.  */
>      KEEP (*crtbegin.o(.ctors))
>      KEEP (*crtbegin?.o(.ctors))
>
> If divergence from other compiler-rt files (e.g.
> libclang_rt.asan-i386.a) is not a concern, clang_rt.i386.crtbegin.o
> should just work with GNU ld.

This seems to imply that the filename convention is not
clang_rt.crtbegin-<target>.o, but instead
clang_rt.<target>.crtbegin.o. I looked for a corresponding Gnu ld
patch but couldn't find one, so it does seem likely that you're using
the latter convention, which just happens to work with the existing
Gnu ld default script.

Can you clarify which of these two naming conventions you intend to
support? If it's the former, this patch is OK with the above change.
If the latter, I think you'll need to adjust this patch.

-cary
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH Gold] Recognize clang-style crtbegin and crtend files

augustine.sterling@gmail.com
On Tue, Jan 7, 2020 at 5:26 PM Cary Coutant <[hidden email]> wrote:
> +    if (*(base_name + match_len) != '.' &&
> +        *(base_name + match_len) != '-')
> +      return false;
>
> Please write these as base_name[match_len].

Updated.

> This seems to imply that the filename convention is not
> clang_rt.crtbegin-<target>.o, but instead
> clang_rt.<target>.crtbegin.o.

To the best of my knowledge, clang never uses this form. I haven't
seen it in the wild.

> Can you clarify which of these two naming conventions you intend to
> support? If it's the former, this patch is OK with the above change.
> If the latter, I think you'll need to adjust this patch.

Clang uses two forms of its own (plus all of the gcc ones), which can
be found in clang/lib/Driver/ToolChain.cpp:416:

$(target_directory)/clang_rt.crtbegin.o
clang_rt.crtbegin-$target.o

This updated patch supports both of those forms.

gold.crt.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH Gold] Recognize clang-style crtbegin and crtend files

Cary Coutant-3
> Clang uses two forms of its own (plus all of the gcc ones), which can
> be found in clang/lib/Driver/ToolChain.cpp:416:
>
> $(target_directory)/clang_rt.crtbegin.o
> clang_rt.crtbegin-$target.o
>
> This updated patch supports both of those forms.

Thanks. This is OK.

-cary