This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Allow mismatched SmallDataLimit and use Min for conflicting values
ClosedPublic

Authored by hiraditya on Aug 4 2022, 7:13 PM.

Details

Summary

Authored by: @joshua-arch1

This patch is to fix an issue about module linking with LTO.

When compiling with PIE, the small data limitation needs to be consistent with that in PIC, otherwise there will be linking errors due to conflicting values.

bar.c

int bar() { return 1; }

foo.c

int foo() { return 1; }
clang --target=riscv64-unknown-linux-gnu -flto -c foo.c -o foo.o -fPIE
clang --target=riscv64-unknown-linux-gnu -flto -c bar.c -o bar.o -fPIC

clang --target=riscv64-unknown-linux-gnu -flto foo.o bar.o -flto -nostdlib -v -fuse-ld=lld
ld.lld: error: linking module flags 'SmallDataLimit': IDs have conflicting values in 'bar.o' and 'ld-temp.o'
clang-15: error: linker command failed with exit code 1 (use -v to see invocation)

What we are trying to do here is to use Min instead of Error for conflicting SmallDataLimit when combining -fno-PIC code with -fPIC code.

Signed-off-by: xiaojing.zhang <xiaojing.zhang@xcalibyte.com>
Signed-off-by: jianxin.lai <jianxin.lai@xcalibyte.com>

Diff Detail

Event Timeline

joshua-arch1 created this revision.Aug 4 2022, 7:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2022, 7:13 PM
joshua-arch1 requested review of this revision.Aug 4 2022, 7:13 PM
jrtc27 added a comment.Aug 4 2022, 8:20 PM

I don’t understand. PIEs can use sdata just fine.

jrtc27 added a comment.Aug 4 2022, 8:21 PM

And why would the existence of .sdata lead to linker errors?

Joshua-401 edited the summary of this revision. (Show Details)Aug 22 2022, 7:01 PM
Joshua-401 edited the summary of this revision. (Show Details)
Joshua-401 edited the summary of this revision. (Show Details)

So your error is about _module_ linking with LTO, not LLD. In that case this doesn't fix the problem, you can still hit it by combining -fno-PIC code with -fPIC code. The correct thing to do is allow mismatched SmallDataLimit and merge them into something sensible (either min or max).

enh added a subscriber: enh.Oct 12 2022, 5:17 PM

@MaskRay for his opinion...

joshua-arch1 edited the summary of this revision. (Show Details)
joshua-arch1 retitled this revision from [RISCV] Setting small data limitation to zero for PIE to [RISCV] Allow mismatched SmallDataLimit and use Min for conflicting values.Nov 3 2022, 1:38 AM
joshua-arch1 edited the summary of this revision. (Show Details)
jrtc27 added a comment.Nov 3 2022, 4:04 AM

Description should say LTO in the first paragraph as otherwise it sounds like normal ELF linking not LLVM module linking. Otherwise the diff looks sensible.

joshua-arch1 edited the summary of this revision. (Show Details)Nov 3 2022, 6:34 PM
joshua-arch1 edited the summary of this revision. (Show Details)

Description should say LTO in the first paragraph as otherwise it sounds like normal ELF linking not LLVM module linking. Otherwise the diff looks sensible.

The description has been updated.

Description should say LTO in the first paragraph as otherwise it sounds like normal ELF linking not LLVM module linking. Otherwise the diff looks sensible.

Ping.

pirama added a subscriber: pirama.Dec 2 2022, 11:02 AM
MaskRay accepted this revision.EditedDec 17 2022, 10:41 PM

Sorry for my belated response. D57497 introduced the Module Flags Metadata "SmallDataLimit". It defaults to 0 for -fPIC and (with some exceptions) otherwise 8.
So a -fno-pic/-fPIE bitcode file and a -fPIC bitcode file cannot be mixed together because "SmallDataLimit" uses the merge behavior Error (all values must match).
There is an overly restricted condition. I agree that lifting it to Min (which is a fairly new addition) makes sense, although I am unsure why SmallDataLimit needs to have a default value of 8 in the first place...

Be sure that @jrtc27 is happy as well.

clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
40–41

If I saw D57497 earlier, I'd suggest adding a prefix for every unique value, instead of using a unique prefix for every test. But you don't need to simplify this.

This revision is now accepted and ready to land.Dec 17 2022, 10:41 PM

Is it okay to merge this patch?

cc: @jrtc27 anything you still want addressed?

Is it okay to merge this patch?

Yes, seems fine to me.

luke957 removed a subscriber: luke957.Jan 28 2023, 11:26 PM

@joshua-arch1 can you share your email such that I can commit this with proper author info? Feel free to ping me directly on discord: Aditya Kumar#9439

Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2023, 3:33 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I have reverted it in 3df16e6f6e4d933f3839003e29b8a4b70e4c7ec8.
Please update CodeGen/RISCV/rvv-intrinsics-handcrafted/vlenb.c and reland this patch again.

thanks for reverting it. i'll take care of the test failure.

hiraditya reopened this revision.Feb 7 2023, 1:58 PM

reopening to update the failing testcase.

This revision is now accepted and ready to land.Feb 7 2023, 1:58 PM
hiraditya commandeered this revision.Feb 7 2023, 1:59 PM
hiraditya updated this revision to Diff 495641.
hiraditya added a reviewer: joshua-arch1.
hiraditya edited the summary of this revision. (Show Details)

Updated the failing testcase.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2023, 5:13 PM

I updated more i32 1, !"SmallDataLimit" and re-landed this patch.