[lld] Merge GOT entries for symbols #131630
Conversation
This becomes a problem for AArch64 when ADRP and LDR instructions are outlined in separate sections. See test case for details.
|
@llvm/pr-subscribers-lld-elf Author: Pranav Kant (pranavk) ChangesThis becomes a problem for AArch64 when ADRP and LDR instructions are outlined in separate sections. See test case for details. Fixes #129122 Full diff: https://github.com/llvm/llvm-project/pull/131630.diff 3 Files Affected:
diff --git a/lld/ELF/SyntheticSections.cpp b/lld/ELF/SyntheticSections.cpp
index b03c4282ab1aa..a22bf2cece61a 100644
--- a/lld/ELF/SyntheticSections.cpp
+++ b/lld/ELF/SyntheticSections.cpp
@@ -667,6 +667,19 @@ GotSection::GotSection(Ctx &ctx)
void GotSection::addConstant(const Relocation &r) { relocations.push_back(r); }
void GotSection::addEntry(const Symbol &sym) {
assert(sym.auxIdx == ctx.symAux.size() - 1);
+ if (auto *d = dyn_cast<Defined>(&sym)) {
+ // There may be symbols that have been ICFed in which case d->section
+ // points to their canonical section and d->value is offset in to that section.
+ // We add only a single GOT entry for all such symbols.
+ auto [it, inserted] = gotEntries.insert(
+ std::make_pair(std::make_pair(d->section, d->value),
+ numEntries));
+ if (!inserted) {
+ ctx.symAux.back().gotIdx = it->getSecond();
+ return;
+ }
+ }
+
ctx.symAux.back().gotIdx = numEntries++;
}
diff --git a/lld/ELF/SyntheticSections.h b/lld/ELF/SyntheticSections.h
index c977562f0b174..3debb08e4dd2b 100644
--- a/lld/ELF/SyntheticSections.h
+++ b/lld/ELF/SyntheticSections.h
@@ -138,6 +138,9 @@ class GotSection final : public SyntheticSection {
bool isSymbolFunc;
};
SmallVector<AuthEntryInfo, 0> authEntries;
+
+ // To track GOT entries for symbols that may have been ICFed
+ llvm::DenseMap<std::pair<SectionBase*, uint64_t>, uint32_t> gotEntries;
};
// .note.GNU-stack section.
diff --git a/lld/test/ELF/aarch64-adrp-ldr-icf.s b/lld/test/ELF/aarch64-adrp-ldr-icf.s
new file mode 100644
index 0000000000000..fa8167cd12c87
--- /dev/null
+++ b/lld/test/ELF/aarch64-adrp-ldr-icf.s
@@ -0,0 +1,1057 @@
+// REQUIRES: aarch64
+
+// RUN: llvm-mc -filetype=obj -triple=aarch64 %s -o %t
+// RUN: ld.lld %t -o %t2 --icf=all
+// RUN: llvm-objdump --section-headers %t2 | FileCheck %s
+
+// CHECK: {{.*}}.got 00000008{{.*}}
+
+.addrsig
+
+callee:
+ret
+
+.section .rodata.dummy1,"a",@progbits
+sym1:
+.long 111
+.long 122
+.byte 123
+
+.section .rodata.dummy2,"a",@progbits
+sym2:
+.long 111
+.long 122
+sym3:
+.byte 123
+
+.macro f, index
+
+.section .text.f1_\index,"ax",@progbits
+f1_\index:
+adrp x0, :got:g\index
+mov x1, #\index
+b f2_\index
+
+.section .text.f2_\index,"ax",@progbits
+f2_\index:
+ldr x0, [x0, :got_lo12:g\index]
+b callee
+
+.section .rodata.g\index,"a",@progbits
+g_\index:
+.long 111
+.long 122
+
+g\index:
+.byte 123
+
+.section .text._start,"ax",@progbits
+bl f1_\index
+
+.endm
+
+.section .text._start,"ax",@progbits
+.globl _start
+_start:
+
+f 0
+f 1
+f 2
+f 3
+f 4
+f 5
+f 6
+f 7
+f 8
+f 9
+f 10
+f 11
+f 12
+f 13
+f 14
+f 15
+f 16
+f 17
+f 18
+f 19
+f 20
+f 21
+f 22
+f 23
+f 24
+f 25
+f 26
+f 27
+f 28
+f 29
+f 30
+f 31
+f 32
+f 33
+f 34
+f 35
+f 36
+f 37
+f 38
+f 39
+f 40
+f 41
+f 42
+f 43
+f 44
+f 45
+f 46
+f 47
+f 48
+f 49
+f 50
+f 51
+f 52
+f 53
+f 54
+f 55
+f 56
+f 57
+f 58
+f 59
+f 60
+f 61
+f 62
+f 63
+f 64
+f 65
+f 66
+f 67
+f 68
+f 69
+f 70
+f 71
+f 72
+f 73
+f 74
+f 75
+f 76
+f 77
+f 78
+f 79
+f 80
+f 81
+f 82
+f 83
+f 84
+f 85
+f 86
+f 87
+f 88
+f 89
+f 90
+f 91
+f 92
+f 93
+f 94
+f 95
+f 96
+f 97
+f 98
+f 99
+f 100
+f 101
+f 102
+f 103
+f 104
+f 105
+f 106
+f 107
+f 108
+f 109
+f 110
+f 111
+f 112
+f 113
+f 114
+f 115
+f 116
+f 117
+f 118
+f 119
+f 120
+f 121
+f 122
+f 123
+f 124
+f 125
+f 126
+f 127
+f 128
+f 129
+f 130
+f 131
+f 132
+f 133
+f 134
+f 135
+f 136
+f 137
+f 138
+f 139
+f 140
+f 141
+f 142
+f 143
+f 144
+f 145
+f 146
+f 147
+f 148
+f 149
+f 150
+f 151
+f 152
+f 153
+f 154
+f 155
+f 156
+f 157
+f 158
+f 159
+f 160
+f 161
+f 162
+f 163
+f 164
+f 165
+f 166
+f 167
+f 168
+f 169
+f 170
+f 171
+f 172
+f 173
+f 174
+f 175
+f 176
+f 177
+f 178
+f 179
+f 180
+f 181
+f 182
+f 183
+f 184
+f 185
+f 186
+f 187
+f 188
+f 189
+f 190
+f 191
+f 192
+f 193
+f 194
+f 195
+f 196
+f 197
+f 198
+f 199
+f 200
+f 201
+f 202
+f 203
+f 204
+f 205
+f 206
+f 207
+f 208
+f 209
+f 210
+f 211
+f 212
+f 213
+f 214
+f 215
+f 216
+f 217
+f 218
+f 219
+f 220
+f 221
+f 222
+f 223
+f 224
+f 225
+f 226
+f 227
+f 228
+f 229
+f 230
+f 231
+f 232
+f 233
+f 234
+f 235
+f 236
+f 237
+f 238
+f 239
+f 240
+f 241
+f 242
+f 243
+f 244
+f 245
+f 246
+f 247
+f 248
+f 249
+f 250
+f 251
+f 252
+f 253
+f 254
+f 255
+f 256
+f 257
+f 258
+f 259
+f 260
+f 261
+f 262
+f 263
+f 264
+f 265
+f 266
+f 267
+f 268
+f 269
+f 270
+f 271
+f 272
+f 273
+f 274
+f 275
+f 276
+f 277
+f 278
+f 279
+f 280
+f 281
+f 282
+f 283
+f 284
+f 285
+f 286
+f 287
+f 288
+f 289
+f 290
+f 291
+f 292
+f 293
+f 294
+f 295
+f 296
+f 297
+f 298
+f 299
+f 300
+f 301
+f 302
+f 303
+f 304
+f 305
+f 306
+f 307
+f 308
+f 309
+f 310
+f 311
+f 312
+f 313
+f 314
+f 315
+f 316
+f 317
+f 318
+f 319
+f 320
+f 321
+f 322
+f 323
+f 324
+f 325
+f 326
+f 327
+f 328
+f 329
+f 330
+f 331
+f 332
+f 333
+f 334
+f 335
+f 336
+f 337
+f 338
+f 339
+f 340
+f 341
+f 342
+f 343
+f 344
+f 345
+f 346
+f 347
+f 348
+f 349
+f 350
+f 351
+f 352
+f 353
+f 354
+f 355
+f 356
+f 357
+f 358
+f 359
+f 360
+f 361
+f 362
+f 363
+f 364
+f 365
+f 366
+f 367
+f 368
+f 369
+f 370
+f 371
+f 372
+f 373
+f 374
+f 375
+f 376
+f 377
+f 378
+f 379
+f 380
+f 381
+f 382
+f 383
+f 384
+f 385
+f 386
+f 387
+f 388
+f 389
+f 390
+f 391
+f 392
+f 393
+f 394
+f 395
+f 396
+f 397
+f 398
+f 399
+f 400
+f 401
+f 402
+f 403
+f 404
+f 405
+f 406
+f 407
+f 408
+f 409
+f 410
+f 411
+f 412
+f 413
+f 414
+f 415
+f 416
+f 417
+f 418
+f 419
+f 420
+f 421
+f 422
+f 423
+f 424
+f 425
+f 426
+f 427
+f 428
+f 429
+f 430
+f 431
+f 432
+f 433
+f 434
+f 435
+f 436
+f 437
+f 438
+f 439
+f 440
+f 441
+f 442
+f 443
+f 444
+f 445
+f 446
+f 447
+f 448
+f 449
+f 450
+f 451
+f 452
+f 453
+f 454
+f 455
+f 456
+f 457
+f 458
+f 459
+f 460
+f 461
+f 462
+f 463
+f 464
+f 465
+f 466
+f 467
+f 468
+f 469
+f 470
+f 471
+f 472
+f 473
+f 474
+f 475
+f 476
+f 477
+f 478
+f 479
+f 480
+f 481
+f 482
+f 483
+f 484
+f 485
+f 486
+f 487
+f 488
+f 489
+f 490
+f 491
+f 492
+f 493
+f 494
+f 495
+f 496
+f 497
+f 498
+f 499
+f 500
+f 501
+f 502
+f 503
+f 504
+f 505
+f 506
+f 507
+f 508
+f 509
+f 510
+f 511
+f 512
+f 513
+f 514
+f 515
+f 516
+f 517
+f 518
+f 519
+f 520
+f 521
+f 522
+f 523
+f 524
+f 525
+f 526
+f 527
+f 528
+f 529
+f 530
+f 531
+f 532
+f 533
+f 534
+f 535
+f 536
+f 537
+f 538
+f 539
+f 540
+f 541
+f 542
+f 543
+f 544
+f 545
+f 546
+f 547
+f 548
+f 549
+f 550
+f 551
+f 552
+f 553
+f 554
+f 555
+f 556
+f 557
+f 558
+f 559
+f 560
+f 561
+f 562
+f 563
+f 564
+f 565
+f 566
+f 567
+f 568
+f 569
+f 570
+f 571
+f 572
+f 573
+f 574
+f 575
+f 576
+f 577
+f 578
+f 579
+f 580
+f 581
+f 582
+f 583
+f 584
+f 585
+f 586
+f 587
+f 588
+f 589
+f 590
+f 591
+f 592
+f 593
+f 594
+f 595
+f 596
+f 597
+f 598
+f 599
+f 600
+f 601
+f 602
+f 603
+f 604
+f 605
+f 606
+f 607
+f 608
+f 609
+f 610
+f 611
+f 612
+f 613
+f 614
+f 615
+f 616
+f 617
+f 618
+f 619
+f 620
+f 621
+f 622
+f 623
+f 624
+f 625
+f 626
+f 627
+f 628
+f 629
+f 630
+f 631
+f 632
+f 633
+f 634
+f 635
+f 636
+f 637
+f 638
+f 639
+f 640
+f 641
+f 642
+f 643
+f 644
+f 645
+f 646
+f 647
+f 648
+f 649
+f 650
+f 651
+f 652
+f 653
+f 654
+f 655
+f 656
+f 657
+f 658
+f 659
+f 660
+f 661
+f 662
+f 663
+f 664
+f 665
+f 666
+f 667
+f 668
+f 669
+f 670
+f 671
+f 672
+f 673
+f 674
+f 675
+f 676
+f 677
+f 678
+f 679
+f 680
+f 681
+f 682
+f 683
+f 684
+f 685
+f 686
+f 687
+f 688
+f 689
+f 690
+f 691
+f 692
+f 693
+f 694
+f 695
+f 696
+f 697
+f 698
+f 699
+f 700
+f 701
+f 702
+f 703
+f 704
+f 705
+f 706
+f 707
+f 708
+f 709
+f 710
+f 711
+f 712
+f 713
+f 714
+f 715
+f 716
+f 717
+f 718
+f 719
+f 720
+f 721
+f 722
+f 723
+f 724
+f 725
+f 726
+f 727
+f 728
+f 729
+f 730
+f 731
+f 732
+f 733
+f 734
+f 735
+f 736
+f 737
+f 738
+f 739
+f 740
+f 741
+f 742
+f 743
+f 744
+f 745
+f 746
+f 747
+f 748
+f 749
+f 750
+f 751
+f 752
+f 753
+f 754
+f 755
+f 756
+f 757
+f 758
+f 759
+f 760
+f 761
+f 762
+f 763
+f 764
+f 765
+f 766
+f 767
+f 768
+f 769
+f 770
+f 771
+f 772
+f 773
+f 774
+f 775
+f 776
+f 777
+f 778
+f 779
+f 780
+f 781
+f 782
+f 783
+f 784
+f 785
+f 786
+f 787
+f 788
+f 789
+f 790
+f 791
+f 792
+f 793
+f 794
+f 795
+f 796
+f 797
+f 798
+f 799
+f 800
+f 801
+f 802
+f 803
+f 804
+f 805
+f 806
+f 807
+f 808
+f 809
+f 810
+f 811
+f 812
+f 813
+f 814
+f 815
+f 816
+f 817
+f 818
+f 819
+f 820
+f 821
+f 822
+f 823
+f 824
+f 825
+f 826
+f 827
+f 828
+f 829
+f 830
+f 831
+f 832
+f 833
+f 834
+f 835
+f 836
+f 837
+f 838
+f 839
+f 840
+f 841
+f 842
+f 843
+f 844
+f 845
+f 846
+f 847
+f 848
+f 849
+f 850
+f 851
+f 852
+f 853
+f 854
+f 855
+f 856
+f 857
+f 858
+f 859
+f 860
+f 861
+f 862
+f 863
+f 864
+f 865
+f 866
+f 867
+f 868
+f 869
+f 870
+f 871
+f 872
+f 873
+f 874
+f 875
+f 876
+f 877
+f 878
+f 879
+f 880
+f 881
+f 882
+f 883
+f 884
+f 885
+f 886
+f 887
+f 888
+f 889
+f 890
+f 891
+f 892
+f 893
+f 894
+f 895
+f 896
+f 897
+f 898
+f 899
+f 900
+f 901
+f 902
+f 903
+f 904
+f 905
+f 906
+f 907
+f 908
+f 909
+f 910
+f 911
+f 912
+f 913
+f 914
+f 915
+f 916
+f 917
+f 918
+f 919
+f 920
+f 921
+f 922
+f 923
+f 924
+f 925
+f 926
+f 927
+f 928
+f 929
+f 930
+f 931
+f 932
+f 933
+f 934
+f 935
+f 936
+f 937
+f 938
+f 939
+f 940
+f 941
+f 942
+f 943
+f 944
+f 945
+f 946
+f 947
+f 948
+f 949
+f 950
+f 951
+f 952
+f 953
+f 954
+f 955
+f 956
+f 957
+f 958
+f 959
+f 960
+f 961
+f 962
+f 963
+f 964
+f 965
+f 966
+f 967
+f 968
+f 969
+f 970
+f 971
+f 972
+f 973
+f 974
+f 975
+f 976
+f 977
+f 978
+f 979
+f 980
+f 981
+f 982
+f 983
+f 984
+f 985
+f 986
+f 987
+f 988
+f 989
+f 990
+f 991
+f 992
+f 993
+f 994
+f 995
+f 996
+f 997
+f 998
+f 999
+f 1000
|
|
@llvm/pr-subscribers-lld Author: Pranav Kant (pranavk) ChangesThis becomes a problem for AArch64 when ADRP and LDR instructions are outlined in separate sections. See test case for details. Fixes #129122 Full diff: https://github.com/llvm/llvm-project/pull/131630.diff 3 Files Affected:
diff --git a/lld/ELF/SyntheticSections.cpp b/lld/ELF/SyntheticSections.cpp
index b03c4282ab1aa..a22bf2cece61a 100644
--- a/lld/ELF/SyntheticSections.cpp
+++ b/lld/ELF/SyntheticSections.cpp
@@ -667,6 +667,19 @@ GotSection::GotSection(Ctx &ctx)
void GotSection::addConstant(const Relocation &r) { relocations.push_back(r); }
void GotSection::addEntry(const Symbol &sym) {
assert(sym.auxIdx == ctx.symAux.size() - 1);
+ if (auto *d = dyn_cast<Defined>(&sym)) {
+ // There may be symbols that have been ICFed in which case d->section
+ // points to their canonical section and d->value is offset in to that section.
+ // We add only a single GOT entry for all such symbols.
+ auto [it, inserted] = gotEntries.insert(
+ std::make_pair(std::make_pair(d->section, d->value),
+ numEntries));
+ if (!inserted) {
+ ctx.symAux.back().gotIdx = it->getSecond();
+ return;
+ }
+ }
+
ctx.symAux.back().gotIdx = numEntries++;
}
diff --git a/lld/ELF/SyntheticSections.h b/lld/ELF/SyntheticSections.h
index c977562f0b174..3debb08e4dd2b 100644
--- a/lld/ELF/SyntheticSections.h
+++ b/lld/ELF/SyntheticSections.h
@@ -138,6 +138,9 @@ class GotSection final : public SyntheticSection {
bool isSymbolFunc;
};
SmallVector<AuthEntryInfo, 0> authEntries;
+
+ // To track GOT entries for symbols that may have been ICFed
+ llvm::DenseMap<std::pair<SectionBase*, uint64_t>, uint32_t> gotEntries;
};
// .note.GNU-stack section.
diff --git a/lld/test/ELF/aarch64-adrp-ldr-icf.s b/lld/test/ELF/aarch64-adrp-ldr-icf.s
new file mode 100644
index 0000000000000..fa8167cd12c87
--- /dev/null
+++ b/lld/test/ELF/aarch64-adrp-ldr-icf.s
@@ -0,0 +1,1057 @@
+// REQUIRES: aarch64
+
+// RUN: llvm-mc -filetype=obj -triple=aarch64 %s -o %t
+// RUN: ld.lld %t -o %t2 --icf=all
+// RUN: llvm-objdump --section-headers %t2 | FileCheck %s
+
+// CHECK: {{.*}}.got 00000008{{.*}}
+
+.addrsig
+
+callee:
+ret
+
+.section .rodata.dummy1,"a",@progbits
+sym1:
+.long 111
+.long 122
+.byte 123
+
+.section .rodata.dummy2,"a",@progbits
+sym2:
+.long 111
+.long 122
+sym3:
+.byte 123
+
+.macro f, index
+
+.section .text.f1_\index,"ax",@progbits
+f1_\index:
+adrp x0, :got:g\index
+mov x1, #\index
+b f2_\index
+
+.section .text.f2_\index,"ax",@progbits
+f2_\index:
+ldr x0, [x0, :got_lo12:g\index]
+b callee
+
+.section .rodata.g\index,"a",@progbits
+g_\index:
+.long 111
+.long 122
+
+g\index:
+.byte 123
+
+.section .text._start,"ax",@progbits
+bl f1_\index
+
+.endm
+
+.section .text._start,"ax",@progbits
+.globl _start
+_start:
+
+f 0
+f 1
+f 2
+f 3
+f 4
+f 5
+f 6
+f 7
+f 8
+f 9
+f 10
+f 11
+f 12
+f 13
+f 14
+f 15
+f 16
+f 17
+f 18
+f 19
+f 20
+f 21
+f 22
+f 23
+f 24
+f 25
+f 26
+f 27
+f 28
+f 29
+f 30
+f 31
+f 32
+f 33
+f 34
+f 35
+f 36
+f 37
+f 38
+f 39
+f 40
+f 41
+f 42
+f 43
+f 44
+f 45
+f 46
+f 47
+f 48
+f 49
+f 50
+f 51
+f 52
+f 53
+f 54
+f 55
+f 56
+f 57
+f 58
+f 59
+f 60
+f 61
+f 62
+f 63
+f 64
+f 65
+f 66
+f 67
+f 68
+f 69
+f 70
+f 71
+f 72
+f 73
+f 74
+f 75
+f 76
+f 77
+f 78
+f 79
+f 80
+f 81
+f 82
+f 83
+f 84
+f 85
+f 86
+f 87
+f 88
+f 89
+f 90
+f 91
+f 92
+f 93
+f 94
+f 95
+f 96
+f 97
+f 98
+f 99
+f 100
+f 101
+f 102
+f 103
+f 104
+f 105
+f 106
+f 107
+f 108
+f 109
+f 110
+f 111
+f 112
+f 113
+f 114
+f 115
+f 116
+f 117
+f 118
+f 119
+f 120
+f 121
+f 122
+f 123
+f 124
+f 125
+f 126
+f 127
+f 128
+f 129
+f 130
+f 131
+f 132
+f 133
+f 134
+f 135
+f 136
+f 137
+f 138
+f 139
+f 140
+f 141
+f 142
+f 143
+f 144
+f 145
+f 146
+f 147
+f 148
+f 149
+f 150
+f 151
+f 152
+f 153
+f 154
+f 155
+f 156
+f 157
+f 158
+f 159
+f 160
+f 161
+f 162
+f 163
+f 164
+f 165
+f 166
+f 167
+f 168
+f 169
+f 170
+f 171
+f 172
+f 173
+f 174
+f 175
+f 176
+f 177
+f 178
+f 179
+f 180
+f 181
+f 182
+f 183
+f 184
+f 185
+f 186
+f 187
+f 188
+f 189
+f 190
+f 191
+f 192
+f 193
+f 194
+f 195
+f 196
+f 197
+f 198
+f 199
+f 200
+f 201
+f 202
+f 203
+f 204
+f 205
+f 206
+f 207
+f 208
+f 209
+f 210
+f 211
+f 212
+f 213
+f 214
+f 215
+f 216
+f 217
+f 218
+f 219
+f 220
+f 221
+f 222
+f 223
+f 224
+f 225
+f 226
+f 227
+f 228
+f 229
+f 230
+f 231
+f 232
+f 233
+f 234
+f 235
+f 236
+f 237
+f 238
+f 239
+f 240
+f 241
+f 242
+f 243
+f 244
+f 245
+f 246
+f 247
+f 248
+f 249
+f 250
+f 251
+f 252
+f 253
+f 254
+f 255
+f 256
+f 257
+f 258
+f 259
+f 260
+f 261
+f 262
+f 263
+f 264
+f 265
+f 266
+f 267
+f 268
+f 269
+f 270
+f 271
+f 272
+f 273
+f 274
+f 275
+f 276
+f 277
+f 278
+f 279
+f 280
+f 281
+f 282
+f 283
+f 284
+f 285
+f 286
+f 287
+f 288
+f 289
+f 290
+f 291
+f 292
+f 293
+f 294
+f 295
+f 296
+f 297
+f 298
+f 299
+f 300
+f 301
+f 302
+f 303
+f 304
+f 305
+f 306
+f 307
+f 308
+f 309
+f 310
+f 311
+f 312
+f 313
+f 314
+f 315
+f 316
+f 317
+f 318
+f 319
+f 320
+f 321
+f 322
+f 323
+f 324
+f 325
+f 326
+f 327
+f 328
+f 329
+f 330
+f 331
+f 332
+f 333
+f 334
+f 335
+f 336
+f 337
+f 338
+f 339
+f 340
+f 341
+f 342
+f 343
+f 344
+f 345
+f 346
+f 347
+f 348
+f 349
+f 350
+f 351
+f 352
+f 353
+f 354
+f 355
+f 356
+f 357
+f 358
+f 359
+f 360
+f 361
+f 362
+f 363
+f 364
+f 365
+f 366
+f 367
+f 368
+f 369
+f 370
+f 371
+f 372
+f 373
+f 374
+f 375
+f 376
+f 377
+f 378
+f 379
+f 380
+f 381
+f 382
+f 383
+f 384
+f 385
+f 386
+f 387
+f 388
+f 389
+f 390
+f 391
+f 392
+f 393
+f 394
+f 395
+f 396
+f 397
+f 398
+f 399
+f 400
+f 401
+f 402
+f 403
+f 404
+f 405
+f 406
+f 407
+f 408
+f 409
+f 410
+f 411
+f 412
+f 413
+f 414
+f 415
+f 416
+f 417
+f 418
+f 419
+f 420
+f 421
+f 422
+f 423
+f 424
+f 425
+f 426
+f 427
+f 428
+f 429
+f 430
+f 431
+f 432
+f 433
+f 434
+f 435
+f 436
+f 437
+f 438
+f 439
+f 440
+f 441
+f 442
+f 443
+f 444
+f 445
+f 446
+f 447
+f 448
+f 449
+f 450
+f 451
+f 452
+f 453
+f 454
+f 455
+f 456
+f 457
+f 458
+f 459
+f 460
+f 461
+f 462
+f 463
+f 464
+f 465
+f 466
+f 467
+f 468
+f 469
+f 470
+f 471
+f 472
+f 473
+f 474
+f 475
+f 476
+f 477
+f 478
+f 479
+f 480
+f 481
+f 482
+f 483
+f 484
+f 485
+f 486
+f 487
+f 488
+f 489
+f 490
+f 491
+f 492
+f 493
+f 494
+f 495
+f 496
+f 497
+f 498
+f 499
+f 500
+f 501
+f 502
+f 503
+f 504
+f 505
+f 506
+f 507
+f 508
+f 509
+f 510
+f 511
+f 512
+f 513
+f 514
+f 515
+f 516
+f 517
+f 518
+f 519
+f 520
+f 521
+f 522
+f 523
+f 524
+f 525
+f 526
+f 527
+f 528
+f 529
+f 530
+f 531
+f 532
+f 533
+f 534
+f 535
+f 536
+f 537
+f 538
+f 539
+f 540
+f 541
+f 542
+f 543
+f 544
+f 545
+f 546
+f 547
+f 548
+f 549
+f 550
+f 551
+f 552
+f 553
+f 554
+f 555
+f 556
+f 557
+f 558
+f 559
+f 560
+f 561
+f 562
+f 563
+f 564
+f 565
+f 566
+f 567
+f 568
+f 569
+f 570
+f 571
+f 572
+f 573
+f 574
+f 575
+f 576
+f 577
+f 578
+f 579
+f 580
+f 581
+f 582
+f 583
+f 584
+f 585
+f 586
+f 587
+f 588
+f 589
+f 590
+f 591
+f 592
+f 593
+f 594
+f 595
+f 596
+f 597
+f 598
+f 599
+f 600
+f 601
+f 602
+f 603
+f 604
+f 605
+f 606
+f 607
+f 608
+f 609
+f 610
+f 611
+f 612
+f 613
+f 614
+f 615
+f 616
+f 617
+f 618
+f 619
+f 620
+f 621
+f 622
+f 623
+f 624
+f 625
+f 626
+f 627
+f 628
+f 629
+f 630
+f 631
+f 632
+f 633
+f 634
+f 635
+f 636
+f 637
+f 638
+f 639
+f 640
+f 641
+f 642
+f 643
+f 644
+f 645
+f 646
+f 647
+f 648
+f 649
+f 650
+f 651
+f 652
+f 653
+f 654
+f 655
+f 656
+f 657
+f 658
+f 659
+f 660
+f 661
+f 662
+f 663
+f 664
+f 665
+f 666
+f 667
+f 668
+f 669
+f 670
+f 671
+f 672
+f 673
+f 674
+f 675
+f 676
+f 677
+f 678
+f 679
+f 680
+f 681
+f 682
+f 683
+f 684
+f 685
+f 686
+f 687
+f 688
+f 689
+f 690
+f 691
+f 692
+f 693
+f 694
+f 695
+f 696
+f 697
+f 698
+f 699
+f 700
+f 701
+f 702
+f 703
+f 704
+f 705
+f 706
+f 707
+f 708
+f 709
+f 710
+f 711
+f 712
+f 713
+f 714
+f 715
+f 716
+f 717
+f 718
+f 719
+f 720
+f 721
+f 722
+f 723
+f 724
+f 725
+f 726
+f 727
+f 728
+f 729
+f 730
+f 731
+f 732
+f 733
+f 734
+f 735
+f 736
+f 737
+f 738
+f 739
+f 740
+f 741
+f 742
+f 743
+f 744
+f 745
+f 746
+f 747
+f 748
+f 749
+f 750
+f 751
+f 752
+f 753
+f 754
+f 755
+f 756
+f 757
+f 758
+f 759
+f 760
+f 761
+f 762
+f 763
+f 764
+f 765
+f 766
+f 767
+f 768
+f 769
+f 770
+f 771
+f 772
+f 773
+f 774
+f 775
+f 776
+f 777
+f 778
+f 779
+f 780
+f 781
+f 782
+f 783
+f 784
+f 785
+f 786
+f 787
+f 788
+f 789
+f 790
+f 791
+f 792
+f 793
+f 794
+f 795
+f 796
+f 797
+f 798
+f 799
+f 800
+f 801
+f 802
+f 803
+f 804
+f 805
+f 806
+f 807
+f 808
+f 809
+f 810
+f 811
+f 812
+f 813
+f 814
+f 815
+f 816
+f 817
+f 818
+f 819
+f 820
+f 821
+f 822
+f 823
+f 824
+f 825
+f 826
+f 827
+f 828
+f 829
+f 830
+f 831
+f 832
+f 833
+f 834
+f 835
+f 836
+f 837
+f 838
+f 839
+f 840
+f 841
+f 842
+f 843
+f 844
+f 845
+f 846
+f 847
+f 848
+f 849
+f 850
+f 851
+f 852
+f 853
+f 854
+f 855
+f 856
+f 857
+f 858
+f 859
+f 860
+f 861
+f 862
+f 863
+f 864
+f 865
+f 866
+f 867
+f 868
+f 869
+f 870
+f 871
+f 872
+f 873
+f 874
+f 875
+f 876
+f 877
+f 878
+f 879
+f 880
+f 881
+f 882
+f 883
+f 884
+f 885
+f 886
+f 887
+f 888
+f 889
+f 890
+f 891
+f 892
+f 893
+f 894
+f 895
+f 896
+f 897
+f 898
+f 899
+f 900
+f 901
+f 902
+f 903
+f 904
+f 905
+f 906
+f 907
+f 908
+f 909
+f 910
+f 911
+f 912
+f 913
+f 914
+f 915
+f 916
+f 917
+f 918
+f 919
+f 920
+f 921
+f 922
+f 923
+f 924
+f 925
+f 926
+f 927
+f 928
+f 929
+f 930
+f 931
+f 932
+f 933
+f 934
+f 935
+f 936
+f 937
+f 938
+f 939
+f 940
+f 941
+f 942
+f 943
+f 944
+f 945
+f 946
+f 947
+f 948
+f 949
+f 950
+f 951
+f 952
+f 953
+f 954
+f 955
+f 956
+f 957
+f 958
+f 959
+f 960
+f 961
+f 962
+f 963
+f 964
+f 965
+f 966
+f 967
+f 968
+f 969
+f 970
+f 971
+f 972
+f 973
+f 974
+f 975
+f 976
+f 977
+f 978
+f 979
+f 980
+f 981
+f 982
+f 983
+f 984
+f 985
+f 986
+f 987
+f 988
+f 989
+f 990
+f 991
+f 992
+f 993
+f 994
+f 995
+f 996
+f 997
+f 998
+f 999
+f 1000
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Please could you write a full description for the commit message? There's a lot of subtle details about why this occurs and why this is the best fix.
It also helps people reading the commit log to not have to refer to an external URL for details.
I'm surprised the outliner has chosen to split apart an ADRP and a ldr. This prevents the relocation optimization
ADRP x0, :got: symbol // R_<CLS>_ADR_GOT_PAGE
LDR x0, [x0 :got_lo12: symbol] // R_<CLS>_LD64_GOT_LO12_NC
// after optimization:
ADRP x0, symbol // R_<CLS>_ADR_PREL_PG_HI21
ADD x0, x0, :lo12: symbol // R_<CLS>_ADD_ABS_LO12_NC
Defined in https://github.com/ARM-software/abi-aa/blob/main/aaelf64/aaelf64.rst#579relocation-optimization
It may be worth raising an issue to stop the outliner from doing that.
lld/test/ELF/aarch64-adrp-ldr-icf.s
Outdated
| b callee | ||
|
|
||
| .section .rodata.g\index,"a",@progbits | ||
| g_\index: |
There was a problem hiding this comment.
g_\index does not appear to be used. I'm guessing you wanted a test to show that if <section, offset> was different then two separate GOT entries were needed?
There was a problem hiding this comment.
Right. I could massage the test a bit more to make sure g_\index is used which would add the GOT entry separately for this.
|
Thanks for taking a look. Yeah, I haven't spent time why outliner is doing that and don't have a repro yet. I will play with it and file an issue. It happens with our internal code. |
lld/ELF/SyntheticSections.cpp
Outdated
| @@ -667,6 +667,20 @@ GotSection::GotSection(Ctx &ctx) | |||
| void GotSection::addConstant(const Relocation &r) { relocations.push_back(r); } | |||
| void GotSection::addEntry(const Symbol &sym) { | |||
| assert(sym.auxIdx == ctx.symAux.size() - 1); | |||
| auto *d = dyn_cast<Defined>(&sym); | |||
| if (d && ctx.arg.icf != ICFLevel::None) { | |||
There was a problem hiding this comment.
Doesn't this also need to check that the symbol is non-preemptible? We don't want to merge GOT entries for preemptible symbols, as that could break preemption semantics.
There was a problem hiding this comment.
From https://github.com/llvm/llvm-project/blob/main/lld/ELF/ICF.cpp#L269 I think that ICF wouldn't fold two sections if the symbols were preemptible.
The code now looks like it only merges entries if the definition is folded which should protect a preemptible symbol, however it looks like we could skip the insertion into gotEntries if the symbol was preemptible.
There was a problem hiding this comment.
That prevents ICF on the sections containing the relocation but not the target sections. Also, it is not necessary for ICF to have been applied to target sections in order to end up with symbols with the same section/value. Consider:
.globl f1, f2
f1:
f2:
ret
We do not want f1 and f2 to have the same GOT entry.
There was a problem hiding this comment.
I think the if (!inserted && d->folded) is an attempt to prevent that case, which I think would work for a single section, but on reflection it isn't sufficient as there could always be ICF'ed sections that d->folded would be true.
With respect to
That prevents ICF on the sections containing the relocation but not the target sections.
Just to check my understanding, this property will prevent sections containing a GOT generating ADRP, or ADD/LDR relocation to a preemptible symbol from being folded. Even if the ADRP is separated into a different section, each "pair" of relocations will refer to the same folded symbol, so the GOT calculations will be correct.
We're left with non-preemptible symbols which no-one should be able to tell the difference if we merge the entries.
One corner case that I haven't thought through yet is CHERI capabilities which write information on the capability to be created at run-time into the GOT slot. These aren't strictly an upstream concern though. I expect that the CHERI fork of LLD already has some code in ICF to avoid merging sections with different derived capabilities (symbol size, type etc.)
There was a problem hiding this comment.
Yes, that d->folded check was an attempt to fix this test case but as I get more understanding of the codebase around, I agree, it's not the correct way to solve the problem here.
There was a problem hiding this comment.
Just to check my understanding, this property will prevent sections containing a GOT generating ADRP, or ADD/LDR relocation to a preemptible symbol from being folded. Even if the ADRP is separated into a different section, each "pair" of relocations will refer to the same folded symbol, so the GOT calculations will be correct.
That's my understanding as well. Symbols are not folded though. Folding happens at section-level. If each symbol is in its own section, their sections will be folded regardless if they contain preemptible or non-preemeptible symbols but any relocations pointing to such symbols won't be folded. Also, relocations are not rewritten. So they keep pointing to same symbols even if the section containing those symbols have been folded into some other section -- that's one of the reason we currently (excluding this patch) create GOT entries for all such g* symbols. We are parsing relocations and calling addGotEntry for every relocation target symbol we encounter. Later those GOT entries get ABS relocation which point to the canonical symbol/section. This way we end up with bunch of GOT entries that all look the same.
lld/ELF/SyntheticSections.cpp
Outdated
| @@ -667,6 +667,20 @@ GotSection::GotSection(Ctx &ctx) | |||
| void GotSection::addConstant(const Relocation &r) { relocations.push_back(r); } | |||
| void GotSection::addEntry(const Symbol &sym) { | |||
| assert(sym.auxIdx == ctx.symAux.size() - 1); | |||
| auto *d = dyn_cast<Defined>(&sym); | |||
| if (d && ctx.arg.icf != ICFLevel::None) { | |||
There was a problem hiding this comment.
From https://github.com/llvm/llvm-project/blob/main/lld/ELF/ICF.cpp#L269 I think that ICF wouldn't fold two sections if the symbols were preemptible.
The code now looks like it only merges entries if the definition is folded which should protect a preemptible symbol, however it looks like we could skip the insertion into gotEntries if the symbol was preemptible.
lld/ELF/SyntheticSections.cpp
Outdated
| auto [it, inserted] = gotEntries.insert( | ||
| std::make_pair(std::make_pair(d->section, d->value), | ||
| numEntries)); | ||
| if (!inserted && d->folded) { |
There was a problem hiding this comment.
Is there a case when a d->folded is seen first and inserted into gotEntries then d->folded == false the canonical definition is encountered and we skip it?
I'm not sure if there's a good one-pass solution to this. Possibly if the value of the map were <Defined, uint32_t> then we could look at the symbol to see if it were folded.
There was a problem hiding this comment.
Yes, I'm not sure why we even need the folded check here. I think we can just let the first symbol we see set the gotIdx.
|
The code looks correct, but can we add a test case showing that preemptible symbols are handled correctly? |
The code still doesn't handle the case you mentioned above:
|
Huh, I'd expect that to be covered by the |
This reverts commit b4c15df.
|
But it's okay for them to have the same GOT entry if they are non-preemptible? Whether that's because they were ICF'd or because they had the same section/value to begin with, it's the same either way. |
|
If both are same kind of symbols, I agree. But now I look closely at the pauth test failure, we need to handle function symbols and data symbols differently as what key we end up using in pauth depends on whether it's a func or not (IA vs DA). |
Also STT_GNU_IFUNC vs STT_FUNC; calling the resolver directly as a normal function might be a bit of an unusual thing to do, but it's perfectly well-defined, and you wouldn't want it to accidentally call the returned function instead (or for IFUNCs to instead run the resolver as the implementation). |
|
I added test case for preemptible symbols and also added a new key to the hashmap that doesn't merge GOT entries for symbols of different types (thanks @jrtc27 ) |
There is a wall of text in the current description. Instead of spending a lot of words explaining the subtle details, it might be clearer to describe the linker output and why it's problematic. (When I read the description, I did not understand what was wrong until I downloaded the example, linked it, and observed the output) (I don't think the current description will help a future reader that stares at the commit message, either) I've left a short description at #129122 (comment) , which could be useful to make the commit message shorter. |
|
This is an intriguing case with some subtle nuances. |
|
Thanks Fangrui for taking a look. We are encountering this in code-size sensitive binaries where using the machine outliner to separate the ADRP and LDR into different sections seems sensible to reduce the binary size. On its own, I don't think it's just an inefficiency -- it's a bug in the linker as it's valid for AArch64 compiler to emit that code and linker should be able to handle that as an input. |
At a minimum, if we're establishing new requirements on input object files, we have to document the new requirements. Ideally, the linker would diagnose invalid inputs, where ADRP/LDR pairs get separated, although this may not be feasible. I think I agree the Machine Outliner should not separate these instructions, because the potential savings from eliminating the load through GOT relaxation are greater than the size savings of outlining. |
|
I chatted more about this Pranav, and I think maybe the right way to think about this change is that it is merging duplicate GOT entries for non-preemptible symbols pointing to the same offset with the same type. If I have 15 protected visibility aliases that all point to the same symbol (no ICF involved), I only need one GOT entry, not 15. This is not just a corner case correctness fix, it's an optimization. To make that clearer, maybe the next step is to move this map of {section, offset, type} to got-index over to Relocations.cpp so that it's clearer what the purpose and lifetime of this data is. |
|
I haven't had time to review the code change here, nor fully digest the ensuing discussion, but the way I look at it is: ICF.cpp should never treat GOT-relocations on symbols as "identical" unless the symbols are folded. Because, they aren't the same. Currently, ICF.cpp just says "these relocations point to the same offset in two sections which can be folded, so therefore the relocations are the same". But they're not! It's an incorrect optimization unless you fold the symbols. So, I think that logic of figuring out which symbols can legitimately be folded, and then doing so, belongs in ICF.cpp rather than GotSection::addEntry. I haven't looked close enough to say how to implement that, but I feel doing it that way would make this change feel a lot less hacky. |
Right. In this case of ICF, it's becoming a correctness issue on top of optimization issue, but in general if you frame it like you framed, it is an optimization opportunity when ICF is not running. I can remove my conditional Regarding your other point of moving it to Relocation.cpp: llvm-project/lld/ELF/Arch/X86_64.cpp Line 351 in 14d2561 gotEntries need to be longer. It seems appropriate to keep this in GotSection class:
I chatted with James offline regarding other solutions but the current patch seems to be the least disruptive -- other alternative solutions like having some sort of symbol folding logic in ICF would add data member to every I can move the condition that checks for |
I see. Reading the phases of Writer.cpp, lots of things happen between relocation scanning (initial GOT entry population) and relaxation, which can create new GOT entries. I guess that's the price of complexity we added for the medium code model. |
|
Pushed a new version of the patch:
|
|
Any comments on the latest patch? I am trying to get this finished by end of this week to unblock one of our internal use case. |
Co-authored-by: Reid Kleckner <rnk@google.com>
| @@ -138,6 +137,14 @@ class GotSection final : public SyntheticSection { | |||
| bool isSymbolFunc; | |||
| }; | |||
| SmallVector<AuthEntryInfo, 0> authEntries; | |||
|
|
|||
| // Map of GOT entries keyed by section, offset, and type. The purpose is to | |||
There was a problem hiding this comment.
How do you propose targets override this to require additional properties be the same? For CHERI targets (currently downstream, but eventually to be upstreamed) we would also need to ensure the symbol size was the same, since that determines the bounds for the GOT entry.
There was a problem hiding this comment.
(along with additional complexity for handling "zero"-sized symbols to ensure that section start symbols get the bounds of the whole section, rather than be 0 bytes as they would otherwise be)
There was a problem hiding this comment.
I imagine in that case the second element of this tuple would evolve into something more complex. Right now it's just the offset. For CHERI targets, it would include size and other things necessary -- first and third element would still be the same.
There was a problem hiding this comment.
But how does that work when the type of the map needs to change? GotSection isn't templated on the target (and refactoring to do so would be a real pain I imagine).
There was a problem hiding this comment.
I think we would be okay if you add another member to the tuple named size (or just create a new struct) and populate that from Symbol::getSize() in GotSection::addEntry(). The original problem we are trying to solve (as mentioned in #129122) would still work.
pranavk
changed the title
There was a problem hiding this comment.
I understand that you've identified an internal issue, invested time in developing a patch, and are eager to move forward with it. The lld patch alone would resolve your issue.
However, I believe prioritizing a fix for MachineOutliner would be a better focus.
I’m still uneasy about merging GOT entries. We should carefully balance code complexity, potential benefits, and the risk of breaking existing functionality. The risk doesn’t seem insignificant, while the payoff appears limited (on x86, it might even be nearly negligible).
| } | ||
| } | ||
|
|
||
| if (!finalGotIdx.has_value()) { |
There was a problem hiding this comment.
we omit braces for single-line simple statements. the code style is concise. we don't add many blank lines.
|
lld on its own should take a valid input and give valid output. If it's valid for Machine outliner to separate out ADRP and LDR in separate sections, lld ought to handle that case. |
This PR adds a GOT-optimization that generates a single GOT entry when same-type, foldable symbols all point to the same location in a section (for ICF, this section could be canonical section folded from multiple sections).
It also fixes a correctness issue for AArch64 when ADRP and LDR instructions are outlined in separate sections and sections are fed to ICF for deduplication.
See test case (based on #129122) for details. All
rodata.*sections are folded into a single section with ICF. This leads to allf2_*function sections getting folded into one (as their relocation target symbolsg*belong to.rodata.g*sections that have already been folded into one). Since relocations still refer originalg*symbols, we end up creating duplicate GOT entry for all such symbols. This PR addresses that by tracking such folded symbols and create one GOT entry for all such symbols.Fixes #129122