This is an archive of the discontinued LLVM Phabricator instance.

[XCOFF] Do not put MergeableCStrings in their own section
ClosedPublic

Authored by w2yehia on Jul 24 2023, 9:44 PM.

Details

Summary

The current implementation generates a csect with a
".rodata.str.x.y" prefix for a MergeableCString variable definition.
However, a reference to such variable does not get the prefix in its
name because there's not enough information in the containing IR;
In particular, without seeing the initializer and absent of some other
indicators, we cannot tell that the referenced variable is a null-
terminated string.

When the AIX codegen in llvm was being developed, the prefixing was copied
from ELF without having the linker take advantage of the info.
Currently, the AIX linker does not have the capability to merge
MergeableCString variables. If such feature would ever get implemented,
the contract between the linker and compiler would have to be reconsidered.

Here's the before and after of this change:

@a = global i64 320255973571806, align 8
@strA = unnamed_addr constant [7 x i8] c"hello\0A\00", align 1  ;; Mergeable1ByteCString
@strB = unnamed_addr constant [8 x i8] c"Blahah\0A\00", align 1 ;; Mergeable1ByteCString
@strC = unnamed_addr constant [2 x i16] [i16 1, i16 0], align 2 ;; Mergeable2ByteCString
@strD = unnamed_addr constant [2 x i16] [i16 1, i16 1], align 2 ;; !isMergeableCString
@strE = external unnamed_addr constant [2 x i16], align 2

-fdata-sections:
  .text  extern        .rodata.str1.1strA        .text  extern        strA
    0    SD       RO                               0    SD       RO
  .text  extern        .rodata.str1.1strB        .text  extern        strB
    0    SD       RO                               0    SD       RO
  .text  extern        .rodata.str2.2strC  ===>  .text  extern        strC
    0    SD       RO                               0    SD       RO
  .text  extern        strD                      .text  extern        strD
    0    SD       RO                               0    SD       RO
  .data  extern        a                         .data  extern        a
    0    SD       RW                               0    SD       RW
  undef  extern        strE                      undef  extern        strE
    0    ER       UA                               0    ER       UA

-fno-data-sections:
  .text  unamex        .rodata.str1.1            .text  unamex        .rodata
    0    SD       RO                               0    SD       RO
  .text  extern        strA                      .text  extern        strA
    0    LD       RO                               0    LD       RO
  .text  extern        strB                      .text  extern        strB
    0    LD       RO                               0    LD       RO
  .text  unamex        .rodata.str2.2      ===>  .text  extern        strC
    0    SD       RO                               0    LD       RO
  .text  extern        strC                      .text  extern        strD
    0    LD       RO                               0    LD       RO
  .text  unamex        .rodata                   .data  unamex        .data
    0    SD       RO                               0    SD       RW
  .text  extern        strD                      .data  extern        a
    0    LD       RO                               0    LD       RW
  .data  unamex        .data                     undef  extern        strE
    0    SD       RW                               0    ER       UA
  .data  extern        a
    0    LD       RW
  undef  extern        strE
    0    ER       UA

Diff Detail

Event Timeline

w2yehia created this revision.Jul 24 2023, 9:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2023, 9:44 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
w2yehia requested review of this revision.Jul 24 2023, 9:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2023, 9:44 PM
w2yehia updated this revision to Diff 544568.Jul 26 2023, 6:11 PM
w2yehia retitled this revision from [XCOFF] Don't prefix the name of a mergable string. to [XCOFF] Do not put MergeableCStrings in their own section.
w2yehia edited the summary of this revision. (Show Details)
daltenty accepted this revision.Jul 28 2023, 9:07 AM

LGTM, with some notes:

This does constitute some kind of ABI change, since we're changing the name of certain symbols which have external linkage (if one module use the codegen before, and another after, the symbol will have different external names in the symbol table).
That said, in practice references would have been broken on the mismatch in name anyhow, as the external name with the extra prefix was already unexpected by the referencing side, so anything this breaks was likely already a problem.

This revision is now accepted and ready to land.Jul 28 2023, 9:07 AM
MaskRay accepted this revision.Jul 28 2023, 9:42 AM
MaskRay added inline comments.
llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-str.ll
22 ↗(On Diff #544568)

Drop unneeded change

This revision was landed with ongoing or failed builds.Jul 28 2023, 8:24 PM
This revision was automatically updated to reflect the committed changes.