On 2018-06-19 18:45, Olivier Hainque wrote: > Hi Rasmus, > > > 1/6: >> vxworks: add target/h/wrn/coreip to the set of system include paths > > As we discussed on the message dedicated to this patch, this > one is ok. > > For the rest, I still have concerns for part of the patches. Some > would be ok modulo minor changes. > > Before we get there: are you familiar with the "munch" facility > of VxWorks ? > > That can offer alternatives to some of the things you were after.
Yes, but doing regexp match on nm output feels wrong (though I am aware that that is how things have been done for decades), and I really prefer using the modern methods, which also allows e.g. use of constructor priorities. > 3/6: >> vxworks: enable use of .init_array/.fini_array for cdtors > > Assuming we agree on the general approach, this part would > be fine modulo adding parens around the expression. It doesn't > do what you think it does in the current form. Ah, of course. I'll add parens for the next roll. > I'm a bit worried that forcing .init_array for constructors > might not work for RTPs, at least on some versions of VxWorks. > > We probably would need to fallback to ".ctors" as the section > base name for not so recent versions. I'm not clear on all the > ramifications there. Hm, but if somebody built a compiler with --enable-initfini-array today and passed -mrtp to it, they would already get .init_array instead of .ctors, no? I.e., initfini-array.h would be included by tm.h after vxworks.h, and the logic there would unconditionally override vxworks.h's setting of TARGET_ASM_CONSTRUCTOR. So this "forcing" seems to happen already for such a compiler, regardless of my patch? > Some of the crt related patches are too potentially damaging > for other modes of possible uses of a VxWorks toolchain, with > possibly other versions of VxWorks. OK. > I understand it works for you, that --enable-initfini-array is > a new mode of use and that users would have the choice of not > configuring with this. > > I still think that we don't want to bind that configuration choice > to something too specific. OK. > The more we enforce from that config choice, the trickier it will > get to be able to understand/explain what it means. The least we enforce, > the easier it will be to maintain and understand, and the more flexible > it will be for users. > > I'll propose alternatives to the crt parts that I'd rather not get > in (below). > > 6/6: >> vxworks: don't define vxworks_asm_out_constructor when using .init_array > > Would be ok together with the previous one, and as long as we don't > uncover fallouts for RTPs (meaning, we might have to revisit this in the > future). See above, I don't see how that could happen. Patch 6/6 is essentially a no-op, except that it makes the compiler binary a tiny bit smaller. > 2/6: >> libgcc: add crt{begin,end} for powerpc-wrs-vxworks target > > That one would be fine as well. Even if we don't immediately add them > to link closures from the config files (see below), I see how handy it > can be to be able to reference them from user specs for instance. Yes, I hope that building and installing crtbegin/end should be harmless; it just requires that crtstuff.c actually compiles cleanly. > 4 and 5/6 >> powerpc/vxworks: add {START,END}FILE_SPEC for HAVE_INITFINI_ARRAY_SUPPORT >> powerpc/vxworks: [hack] add crti.o file > > I really think those two should stay out. As I mentioned in a previous > message, > there are many possible uses of a toolchain and this (unconditional addition > of crtfiles and references to _ctors) would undoubtedly cause trouble in one > case or another. > > Also, the reference to _ctors tie to strong assumptions about how the > environment arranges for .init_array to work, so really blurs the > responsibility > assignments. > > We might be able to come up with a proper set of options controlling what > crt file to include in such or such circumstance but we're not quite there > yet. > > In the meantime, I think you could get things to work with a custom > userland spec and local crti.o (or alike, on your end so you can name and > locate it as you wish). Yes, these two are the most hackish ones, and I don't much like them myself. So I'm happy to leave them out of mainline, and just use them (or something equivalent) in-house. > You could have, for example a spec file like this (sketch, to give an idea): > > *startfile: > + vx_ctors-begin.o crtbegin.o%s > > where vx_ctors-begin.c would hold something like: > > void (* volatile _ctors[])() > __attribute__((section (".init_array"))) = {}; > > with possible adjustments to priority sections and synchronization > with your linker script. > > You can handle the sentinel addition likewise, with a '*endfile' > spec, or leave this to your linker script. Using a custom spec file, custom linker script and something like the vx_ctors_begin.o will probably be the long-term solution for us. We'd be really happy to be able to use an upstream gcc out-of-the-box apart from those few pieces of glue. I'll fix and resend 1,2,3,6. Once again, thank you for you time, Olivier! Rasmus