Page MenuHomePhabricator

[RISCV][crt] support building without init_array
ClosedPublic

Authored by EccoTheDolphin on Sep 20 2020, 10:39 PM.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2020, 10:39 PM
Herald added subscribers: Restricted Project, evandro, luismarques and 11 others. · View Herald Transcript
EccoTheDolphin requested review of this revision.Sep 20 2020, 10:39 PM
vitalybuka accepted this revision.Sep 21 2020, 1:39 AM
This revision is now accepted and ready to land.Sep 21 2020, 1:39 AM

rebasing patchset...

RISC-V LLVM and GCC are default using init_array, curious why we need this?

RISC-V LLVM and GCC are default using init_array, curious why we need this?

Actually, yes. You are correct - this one is not needed for ASAN support... My understanding is that this may be needed if we link against some custom runtime. I'll adjust the description of the patch.

vitalybuka resigned from this revision.Sep 24 2020, 1:30 AM

Actually I not appropriate reviewer for this one

This revision now requires review to proceed.Sep 24 2020, 1:30 AM

RISC-V LLVM and GCC are default using init_array, curious why we need this?

Actually, yes. You are correct - this one is not needed for ASAN support... My understanding is that this may be needed if we link against some custom runtime. I'll adjust the description of the patch.

I would prefer let those custom runtime to using their crt files, otherwise that add extra 18 byte(auipc+jal+auipc+jal+c.nop) or 20 byte (auipc+jal+auipc+jal+nop) for all RISC-V program which using compiler-rt.

RISC-V LLVM and GCC are default using init_array, curious why we need this?

Actually, yes. You are correct - this one is not needed for ASAN support... My understanding is that this may be needed if we link against some custom runtime. I'll adjust the description of the patch.

I would prefer let those custom runtime to using their crt files, otherwise that add extra 18 byte(auipc+jal+auipc+jal+c.nop) or 20 byte (auipc+jal+auipc+jal+nop) for all RISC-V program which using compiler-rt.

But this logic is enabled only if CRT_HAS_INITFINI_ARRAY is not defined. Which is controlled by cmake (configuration script). In addition, other architectures (x86, sparc, arm, aarch64, sparc, powerpc) - do allow such fallback option. It just seems strange that RISCV does not have one.

kito-cheng accepted this revision.Sep 24 2020, 2:33 AM

But this logic is enabled only if CRT_HAS_INITFINI_ARRAY is not defined. Which is controlled by cmake (configuration script). In addition, other architectures (x86, sparc, arm, aarch64, sparc, powerpc) - do allow such fallback option. It just seems strange that RISCV does not have one.

Oh, I didn't noticed that #elif is associated with #ifdef CRT_HAS_INITFINI_ARRAY, thanks your clarify, that's LGTM now.

This revision is now accepted and ready to land.Sep 24 2020, 2:33 AM
phosek accepted this revision.Sep 24 2020, 11:24 AM

LGTM

compiler-rt/lib/crt/crtbegin.c
120

Do you need the nop here? It's not used for __do_init call.

luismarques accepted this revision.Oct 7 2020, 6:23 AM

LGTM. Just remove nop, ensure it isn't actually needed (rerun the tests, etc.), and feel free to commit this patch. Thanks!

compiler-rt/lib/crt/crtbegin.c
120

I agree the nop doesn't seem to be needed, and so should be removed. I'm not sure why it was added to the powerpc implementation, but I suspect that it's for branch relaxation.

@phosek, @luismarques thanks for your feedback (sorry for the delay with reply)!

Yes, the nop is not really needed. I'll make appropriate adjustments.

@phosek, @luismarques thanks for your feedback (sorry for the delay with reply)!

Yes, the nop is not really needed. I'll make appropriate adjustments.

@EccoTheDolphin ping?

removing unnecessary nop

This revision was landed with ongoing or failed builds.Dec 1 2020, 6:18 AM
This revision was automatically updated to reflect the committed changes.
MaskRay added a subscriber: MaskRay.Dec 1 2020, 2:10 PM

This does not look right to me. Newer architectures like aarch64/risc-v do not need to have .ctors/.dtors support. Can you investigate why -DCRT_HAS_INITFINI_ARRAY isn't set in your build instead?

This does not look right to me. Newer architectures like aarch64/risc-v do not need to have .ctors/.dtors support. Can you investigate why -DCRT_HAS_INITFINI_ARRAY isn't set in your build instead?

ping @EccoTheDolphin?