Skip to content

Remove deprecated ld option (--add-needed)#9141

Merged
masaori335 merged 1 commit intoapache:masterfrom
masaori335:fix-lld
Oct 24, 2022
Merged

Remove deprecated ld option (--add-needed)#9141
masaori335 merged 1 commit intoapache:masterfrom
masaori335:fix-lld

Conversation

@masaori335
Copy link
Contributor

@masaori335 masaori335 commented Oct 13, 2022

While I'm trying mimalloc (and jemalloc) with llvm toolchain (lld), I got the below error.

ld.lld: error: unknown argument '--add-needed'

It looks like lld doesn't support this option. Also ld deprecated this option.

--add-needed
--no-add-needed
These two options have been deprecated because of the similarity of their names to the --as-needed and --no-as-needed options. They have been replaced by --copy-dt-needed-entries and --no-copy-dt-needed-entries.

https://sourceware.org/binutils/docs/ld/Options.html

@masaori335 masaori335 added the Build work related to build configuration or environment label Oct 13, 2022
@masaori335 masaori335 requested a review from duke8253 October 13, 2022 07:23
@masaori335 masaori335 self-assigned this Oct 13, 2022
masaori335
masaori335 commented Oct 13, 2022
View reviewed changes
build/jemalloc.m4
if test "$jemalloc_base_dir" != "/usr"; then
TS_ADDTO(CPPFLAGS, [-I${jemalloc_include}])
TS_ADDTO(LDFLAGS, [-L${jemalloc_ldflags}])
TS_ADDTO(LDFLAGS, [-Wl,--add-needed -L${jemalloc_ldflags} -Wl,-rpath,${jemalloc_ldflags} -Wl,--no-as-needed])
Copy link
Contributor Author

@masaori335 masaori335 Oct 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@duke8253 is this typo of --as-needed option? Because this line has --no-as-needed option at the end and it restores the default behavior.

If we really need --add-needed option here, --copy-dt-needed-entries is the replacement, but lld doesn't have it either.

Copy link
Contributor

@duke8253 duke8253 Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a typo because I remember coming back to this and thought it was a typo, but changing it broke stuff. Though I can't remember exactly what was it now, vaguely recall that something about the order of specifying those options made a difference.

@masaori335
Copy link
Contributor Author

masaori335 commented Oct 13, 2022

[approve ci fedora]

@randall
Copy link
Contributor

randall commented Oct 13, 2022 •
edited
Loading

I'm +1 on this(and had tried to do it myself), but reverted it in #8533

Locally, I just added sed -i -e 's/ -Wl,--no-as-needed//g' build/jemalloc.m4 in the spec file to work around this.

We should probably check if lld is being used and not and these flags unconditionally.

@masaori335
Copy link
Contributor Author

masaori335 commented Oct 14, 2022

I checked with the devtoolset-9 on centos7 (similar env of #8529), it looks like this change works with ld.

configure:30542: cc -o conftest  -D_GNU_SOURCE -DOPENSSL_NO_SSL_INTERN -I/opt/jemalloc/include   -L/opt/jemalloc/lib -Wl,--as-needed -Wl,-rpath,/opt/jemalloc/lib -Wl,--no-as-needed conftest.c -ljemalloc  -lpthread -ldl  >&5
configure:30542: $? = 0
configure:30559: result: -ljemalloc
configure:30573: checking jemalloc/jemalloc.h usability
configure:30573: cc -c  -D_GNU_SOURCE -DOPENSSL_NO_SSL_INTERN -I/opt/jemalloc/include conftest.c >&5
configure:30573: $? = 0
configure:30573: result: yes
...
LD='/opt/rh/devtoolset-9/root/usr/libexec/gcc/x86_64-redhat-linux/9/ld -m elf_x86_64'

@bryancall bryancall added this to the 10.0.0 milestone Oct 17, 2022
@duke8253
Copy link
Contributor

duke8253 commented Oct 20, 2022

Yeah, at the time there were some weird build errors on our centos test boxes. I'll find some time tomorrow to build it on there with the changes to see if things are working.

duke8253
duke8253 approved these changes Oct 24, 2022
View reviewed changes
Copy link
Contributor

@duke8253 duke8253 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine in my testings.

@masaori335 masaori335 merged commit 875c73e into apache:master Oct 24, 2022
zwoop pushed a commit that referenced this pull request Nov 1, 2022
@masaori335 @zwoop
Remove deprecated ld option (--add-needed) (#9141)
c9d6e99 
(cherry picked from commit 875c73e)
@zwoop
Copy link
Contributor

zwoop commented Nov 1, 2022

Cherry-picked to v9.2.x

@zwoop zwoop modified the milestones: 10.0.0, 9.2.0 Nov 1, 2022
SolidWallOfCode pushed a commit to SolidWallOfCode/trafficserver that referenced this pull request Nov 15, 2022
@masaori335 @SolidWallOfCode
Remove deprecated ld option (--add-needed) (apache#9141)
392eb83
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request Feb 21, 2023
@zwoop
Merge remote-tracking branch 'asf/9.2.x' into 9.2.x
d7c5131 
* asf/9.2.x:
  Updated ChangeLog
  Add docs for strategies.yaml hash_string (apache#9026)
  Fix hosting.config reloading (apache#9046)
  Remove unnecessary, dangerous casts from SET_HANDLER and SET_CONTINUATION invocations. (apache#9129)
  Remove deprecated ld option (--add-needed) (apache#9141)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@duke8253 duke8253 duke8253 approved these changes

Assignees

@masaori335 masaori335

Labels

Build work related to build configuration or environment

Projects

None yet

Milestone

9.2.0

Development

Successfully merging this pull request may close these issues.

5 participants

@masaori335 @randall @duke8253 @zwoop @bryancall