Page MenuHomePhabricator

Fix -gz=zlib options for linker
ClosedPublic

Authored by yaxunl on Sep 8 2020, 12:05 PM.

Details

Summary

gcc translates -gz=zlib to --compress-debug-options=zlib for both assembler and linker
but clang only does this for assembler.

The linker needs --compress-debug-options=zlib option to compress the debug sections
in the generated executable or shared library.

Due to this bug, -gz=zlib has no effect on the generated executable or shared library.

This patch fixes that.

Diff Detail

Event Timeline

yaxunl created this revision.Sep 8 2020, 12:05 PM
yaxunl requested review of this revision.Sep 8 2020, 12:05 PM
MaskRay added inline comments.Sep 8 2020, 12:19 PM
clang/test/Driver/gz.c
1 ↗(On Diff #290560)

This can be merged into compress.c
You may delete some -c from compress.c to exercise the linker stage.

clang/test/Driver/hip-gz-options.hip
2

REQUIRES values can be comma separated

6

Is -target (or --target=) needed?

16

Delete trailing empty lines

MaskRay added inline comments.Sep 8 2020, 12:22 PM
clang/test/Driver/hip-gz-options.hip
7

-ggdb is unrelated

7

Sorry it is related.

yaxunl updated this revision to Diff 291227.Sep 11 2020, 8:27 AM
yaxunl marked 6 inline comments as done.

revised by Fangrui's comments.

yaxunl added inline comments.Sep 11 2020, 8:32 AM
clang/test/Driver/gz.c
1 ↗(On Diff #290560)

done

clang/test/Driver/hip-gz-options.hip
2

merged

6

yes. added

16

done

MaskRay added inline comments.Sep 11 2020, 9:18 AM
clang/test/Driver/compress.c
34

You can delete -fintegrated-as since it is the default for most targets/OSes.

39

If the user sets CLANG_DEFAULT_LINKER to a string not containing "ld", this line will not match. Matching a different substring on the linker command line can be more robust, e.g. "-m" "elf_x86_64"

43

Delete -fintegrated-as if it is the default on amdgcn-amd-amdhsa

yaxunl updated this revision to Diff 291285.Sep 11 2020, 11:16 AM
yaxunl marked 3 inline comments as done.

fix tests

clang/test/Driver/compress.c
39

"-m" "elf_x86_64" only works for x86_64, but we also have amdgcn target.

I will just remove "ld".

MaskRay added inline comments.Sep 11 2020, 11:23 AM
clang/test/Driver/hip-gz-options.hip
2

clang-driver is for some legacy cygwin stuff. It should not be needed.

Does --offload-arch=gfx906 have special requirement that the backend is needed? For most other driver tests x86-registered-target is not needed to test %clang -target x86_64

13

Do the two patterns require CHECK-DAG?

15

This ".*ld.*" needs to be removed as well

yaxunl updated this revision to Diff 291310.Sep 11 2020, 12:45 PM
yaxunl marked 3 inline comments as done.

fix tests

clang/test/Driver/hip-gz-options.hip
2

This may be unnecessary. I will remove it.

On the other hand, I think it requires zlib, so I will add zlib.

Also I separated the test for target amdgcn from compress.c since it requires amdgpu-registered-target.

13

only the middle two steps need CHECK-DAG

MaskRay accepted this revision.Sep 11 2020, 12:51 PM
MaskRay added inline comments.
clang/test/Driver/hip-gz-options.hip
2

LG. clang-driver can still be removed

15

Since this is no longer a regex you can remove the surround {{ }}

This revision is now accepted and ready to land.Sep 11 2020, 12:51 PM
yaxunl marked 2 inline comments as done.Sep 11 2020, 12:56 PM
yaxunl added inline comments.
clang/test/Driver/hip-gz-options.hip
2

will do when committing

15

will do when committing

This revision was automatically updated to reflect the committed changes.
yaxunl marked 2 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptSep 11 2020, 2:13 PM

This broke the PPC LLD bot and the failure has been ignored for 4 days. I believe it should be fixed with 3bc3983f229.

This broke the PPC LLD bot and the failure has been ignored for 4 days. I believe it should be fixed with 3bc3983f229.

Sorry I thought I have fixed that but forgot to check. Not sure if this will cause regression on cygwin or msvc. If so pls let me know. Thanks.