[Hexagon] Small data constants cause duplicate symbols

[alexrp]

When building the Zig test suite for Hexagon, we see lots of errors like this:

error: ld.lld: duplicate symbol: .CONST_0000000000000100
    note: defined at test
    note:            /home/alexrp/Source/ziglang/zig/.zig-cache/o/8c04f039e5f8745b4f33232b0a957843/test.o:(.gnu.linkonce.l8.CONST_0000000000000100+0x0)
    note: defined at compiler_rt
    note:            /home/alexrp/.cache/zig/o/9c7be22a9275891985506bbdb2ae4ceb/libcompiler_rt.a.o:(.gnu.linkonce.l8.CONST_0000000000000100+0x0) in archive /home/alexrp/.cache/zig/o/9c7be22a9275891985506bbdb2ae4ceb/libcompiler_rt.a
error: ld.lld: duplicate symbol: .CONST_3FF0000000000000
    note: defined at test
    note:            /home/alexrp/Source/ziglang/zig/.zig-cache/o/8c04f039e5f8745b4f33232b0a957843/test.o:(.gnu.linkonce.l8.CONST_3FF0000000000000+0x0)
    note: defined at compiler_rt
    note:            /home/alexrp/.cache/zig/o/9c7be22a9275891985506bbdb2ae4ceb/libcompiler_rt.a.o:(.gnu.linkonce.l8.CONST_3FF0000000000000+0x0) in archive /home/alexrp/.cache/zig/o/9c7be22a9275891985506bbdb2ae4ceb/libcompiler_rt.a

I find the MCSA_Global in these places pretty suspicious:

llvm-project/llvm/lib/Target/Hexagon/AsmParser/HexagonAsmParser.cpp

Line 1567 in 55fa7e1

getStreamer().emitSymbolAttribute(Sym, MCSA_Global);

llvm-project/llvm/lib/Target/Hexagon/HexagonAsmPrinter.cpp

Line 208 in 55fa7e1

OutStreamer.emitSymbolAttribute(Sym, MCSA_Global);

Shouldn't these be MCSA_Local? (Note that LLD does not have special support for .gnu.linkonce.*.)

We've worked around this in Zig for now by always disabling the small-data feature, but we'd like to get rid of that hack.

cc @androm3da

[llvmbot]

@llvm/issue-subscribers-backend-hexagon

Author: Alex Rønne Petersen (alexrp)

When building the Zig test suite for Hexagon, we see lots of errors like this:

error: ld.lld: duplicate symbol: .CONST_0000000000000100
    note: defined at test
    note:            /home/alexrp/Source/ziglang/zig/.zig-cache/o/8c04f039e5f8745b4f33232b0a957843/test.o:(.gnu.linkonce.l8.CONST_0000000000000100+0x0)
    note: defined at compiler_rt
    note:            /home/alexrp/.cache/zig/o/9c7be22a9275891985506bbdb2ae4ceb/libcompiler_rt.a.o:(.gnu.linkonce.l8.CONST_0000000000000100+0x0) in archive /home/alexrp/.cache/zig/o/9c7be22a9275891985506bbdb2ae4ceb/libcompiler_rt.a
error: ld.lld: duplicate symbol: .CONST_3FF0000000000000
    note: defined at test
    note:            /home/alexrp/Source/ziglang/zig/.zig-cache/o/8c04f039e5f8745b4f33232b0a957843/test.o:(.gnu.linkonce.l8.CONST_3FF0000000000000+0x0)
    note: defined at compiler_rt
    note:            /home/alexrp/.cache/zig/o/9c7be22a9275891985506bbdb2ae4ceb/libcompiler_rt.a.o:(.gnu.linkonce.l8.CONST_3FF0000000000000+0x0) in archive /home/alexrp/.cache/zig/o/9c7be22a9275891985506bbdb2ae4ceb/libcompiler_rt.a

I find the MCSA_Global in these places pretty suspicious:

llvm-project/llvm/lib/Target/Hexagon/AsmParser/HexagonAsmParser.cpp

Line 1567 in 55fa7e1

getStreamer().emitSymbolAttribute(Sym, MCSA_Global);

llvm-project/llvm/lib/Target/Hexagon/HexagonAsmPrinter.cpp

Line 208 in 55fa7e1

OutStreamer.emitSymbolAttribute(Sym, MCSA_Global);

Shouldn't these be MCSA_Local?

We've worked around this in Zig for now by always disabling the small-data feature.

cc @androm3da

[quic-seaswara]

You might want to disable small data (or) use https://github.com/qualcomm/eld here?

[androm3da]

You might want to disable small data (or) use https://github.com/qualcomm/eld here?

Sounds like that's the existing workaround:

We've worked around this in Zig for now by always disabling the small-data feature.

I wonder if it might make sense for the backend to small-data to be off by default?

diff --git a/llvm/lib/Target/Hexagon/Hexagon.td b/llvm/lib/Target/Hexagon/Hexagon.td
index 0dbe743d13ed..b9bd9a2a8e47 100644
--- a/llvm/lib/Target/Hexagon/Hexagon.td
+++ b/llvm/lib/Target/Hexagon/Hexagon.td
@@ -106,7 +106,7 @@ def FeatureNVJ: SubtargetFeature<"nvj", "UseNewValueJumps", "true",
       "Support for new-value jumps", [FeaturePackets]>;
 def FeatureNVS: SubtargetFeature<"nvs", "UseNewValueStores", "true",
       "Support for new-value stores", [FeaturePackets]>;
-def FeatureSmallData: SubtargetFeature<"small-data", "UseSmallData", "true",
+def FeatureSmallData: SubtargetFeature<"small-data", "UseSmallData", "false",
       "Allow GP-relative addressing of global variables">;
 def FeatureDuplex: SubtargetFeature<"duplex", "EnableDuplex", "true",
       "Enable generation of duplex instruction">;

clang's driver could have a setting that preserves the existing behavior of -G0, etc.

[alexrp]

I think it's generally a good idea for LLVM backend defaults to match the capabilities of LLD, FWIW.

[androm3da]

I think it's generally a good idea for LLVM backend defaults to match the capabilities of LLD, FWIW.

IIRC the compiler driver and backend behavior with respect to small-data were written prior to lld existing at all, much less before the hexagon support was added to it.

When the hexagon toolchain is used in commercial scenarios it's generally paired with eld or its closed-source predecessor. So that means that the use cases driving many bugfixes and features have been focused on that deployment for a long time.

lld is not picked by the driver as the default linker for many/most llvm based toolchains - certainly not native linux ones. So I'm not so sure that there really is this trend of default-matching among backend/lld, is there?

That said: we can and should improve the level of support for the hexagon architecture in lld. But regardless of whether lld gains this feature or even if lld had never had any hexagon support, it might still be a good idea for the compiler backend to have a saner default for small-data here.

[alexrp]

lld is not picked by the driver as the default linker for many/most llvm based toolchains - certainly not native linux ones. So I'm not so sure that there really is this trend of default-matching among backend/lld, is there?

That's right. But I do think there's value in ensuring that, for targets where both LLVM and LLD support exists, the two can be used together by default with no major caveats; in general, it makes for a much nicer experience as a user of LLVM when all the different components of the toolchain Just Work with each other. It may not be the result of any conscious decisions on the part of the maintainers, but that's been our experience with most LLVM backends.