Re: CVS commit: src/usr.bin/crunch/crunchgen
On Mon, Dec 23, 2019 at 06:54:47PM -, Christos Zoulas wrote: > Here is a patch that: > > 1. removes all the special handling of variables (-d -p -P -s -S) that >were dealing with DBG (-d) LDSTATIC/NOPIE (-p), and the rest with >disabling/enabling sanitizers. > 2. uses emalloc/estrdup for all the allocators instead of only some cases. > 3. adds -V varspec which passes variables on the command line (as DBG >and LDSTATIC used to be passed before) instead of appending them >to the on-the-fly Makefile using -v varspec. > > The motivation of this is to make variable handling consistent, less magical, > and remove the need for changing crunchgen each time we want to add disabling > an option by default. I like this in principle, but I haven't double checked everything. Joerg
Re: CVS commit: src/usr.bin/crunch/crunchgen
On Thu, Dec 19, 2019 at 12:27:23PM -0500, Christos Zoulas wrote: > On Dec 19, 4:19pm, jo...@bec.de (Joerg Sonnenberger) wrote: > -- Subject: Re: CVS commit: src/usr.bin/crunch/crunchgen > > | > I think that there are two uses: install media and rescue. I can reinstate > | > it for rescue. Is that ok? Note that by default both ssp and fortify are > | > off for most things, so my change was mostly a no-op for the default > settings. > | > | We seem to default to off for more things, PIE too for example. > > Indeed, I also dislike the alphabet soup of flags for disabling > things. I propose that instead we provide a way to add a preamble > to the generated Makefile via -f file, default to a > either a built-in configuration file, or one installed in the tree. > It is inappropriate for crunchgen to have knowledge of variables > specific to our build system that change over time. > > So I propose: > > 1. to make that change [configuration file] > 2. leave the defaults as they are for the installation binaries > 3. change the defaults to turn nothing off for the rescue binaries With the exception of the default, I agree. IMO crunchgen is a general purpose tool. It is used in different circumstances and shouldn't dictate whether "normal" use is space constrained or not. Joerg
Re: CVS commit: src/usr.bin/crunch/crunchgen
On Dec 19, 4:19pm, jo...@bec.de (Joerg Sonnenberger) wrote: -- Subject: Re: CVS commit: src/usr.bin/crunch/crunchgen | > I think that there are two uses: install media and rescue. I can reinstate | > it for rescue. Is that ok? Note that by default both ssp and fortify are | > off for most things, so my change was mostly a no-op for the default settings. | | We seem to default to off for more things, PIE too for example. Indeed, I also dislike the alphabet soup of flags for disabling things. I propose that instead we provide a way to add a preamble to the generated Makefile via -f file, default to a either a built-in configuration file, or one installed in the tree. It is inappropriate for crunchgen to have knowledge of variables specific to our build system that change over time. So I propose: 1. to make that change [configuration file] 2. leave the defaults as they are for the installation binaries 3. change the defaults to turn nothing off for the rescue binaries christos
Re: CVS commit: src/usr.bin/crunch/crunchgen
On Thu, Dec 19, 2019 at 01:46:43PM -, Christos Zoulas wrote: > In article <20191218222723.ga17...@bec.de>, > Joerg Sonnenberger wrote: > >On Wed, Dec 18, 2019 at 03:51:21PM -, Christos Zoulas wrote: > >> In article <20191218152113.ga7...@bec.de>, > >> Joerg Sonnenberger wrote: > >> >On Tue, Dec 17, 2019 at 09:16:04PM -0500, Christos Zoulas wrote: > >> >> Module Name:src > >> >> Committed By: christos > >> >> Date: Wed Dec 18 02:16:04 UTC 2019 > >> >> > >> >> Modified Files: > >> >> src/usr.bin/crunch/crunchgen: crunchgen.1 crunchgen.c > >> >> > >> >> Log Message: > >> >> Also disable ssp and fortify by default. > >> > > >> >Why? > >> > >> Size reduction on install media. There are flags to turn it on. > > > >Not all users of crunchgen are space constrained, so this seems like a > >bad default to me. > > I think that there are two uses: install media and rescue. I can reinstate > it for rescue. Is that ok? Note that by default both ssp and fortify are > off for most things, so my change was mostly a no-op for the default settings. We seem to default to off for more things, PIE too for example. Joerg
Re: CVS commit: src/usr.bin/crunch/crunchgen
In article <20191218222723.ga17...@bec.de>, Joerg Sonnenberger wrote: >On Wed, Dec 18, 2019 at 03:51:21PM -, Christos Zoulas wrote: >> In article <20191218152113.ga7...@bec.de>, >> Joerg Sonnenberger wrote: >> >On Tue, Dec 17, 2019 at 09:16:04PM -0500, Christos Zoulas wrote: >> >> Module Name: src >> >> Committed By: christos >> >> Date: Wed Dec 18 02:16:04 UTC 2019 >> >> >> >> Modified Files: >> >> src/usr.bin/crunch/crunchgen: crunchgen.1 crunchgen.c >> >> >> >> Log Message: >> >> Also disable ssp and fortify by default. >> > >> >Why? >> >> Size reduction on install media. There are flags to turn it on. > >Not all users of crunchgen are space constrained, so this seems like a >bad default to me. I think that there are two uses: install media and rescue. I can reinstate it for rescue. Is that ok? Note that by default both ssp and fortify are off for most things, so my change was mostly a no-op for the default settings. christos
Re: CVS commit: src/usr.bin/crunch/crunchgen
On Wed, Dec 18, 2019 at 03:51:21PM -, Christos Zoulas wrote: > In article <20191218152113.ga7...@bec.de>, > Joerg Sonnenberger wrote: > >On Tue, Dec 17, 2019 at 09:16:04PM -0500, Christos Zoulas wrote: > >> Module Name: src > >> Committed By: christos > >> Date: Wed Dec 18 02:16:04 UTC 2019 > >> > >> Modified Files: > >>src/usr.bin/crunch/crunchgen: crunchgen.1 crunchgen.c > >> > >> Log Message: > >> Also disable ssp and fortify by default. > > > >Why? > > Size reduction on install media. There are flags to turn it on. Not all users of crunchgen are space constrained, so this seems like a bad default to me. Joerg
Re: CVS commit: src/usr.bin/crunch/crunchgen
In article <20191218152113.ga7...@bec.de>, Joerg Sonnenberger wrote: >On Tue, Dec 17, 2019 at 09:16:04PM -0500, Christos Zoulas wrote: >> Module Name: src >> Committed By:christos >> Date:Wed Dec 18 02:16:04 UTC 2019 >> >> Modified Files: >> src/usr.bin/crunch/crunchgen: crunchgen.1 crunchgen.c >> >> Log Message: >> Also disable ssp and fortify by default. > >Why? Size reduction on install media. There are flags to turn it on. christos
Re: CVS commit: src/usr.bin/crunch/crunchgen
On Tue, Dec 17, 2019 at 09:16:04PM -0500, Christos Zoulas wrote: > Module Name: src > Committed By: christos > Date: Wed Dec 18 02:16:04 UTC 2019 > > Modified Files: > src/usr.bin/crunch/crunchgen: crunchgen.1 crunchgen.c > > Log Message: > Also disable ssp and fortify by default. Why? Joerg
Re: CVS commit: src/usr.bin/crunch/crunchgen
On Wed, Feb 13, 2019 at 07:56:26AM +0100, Martin Husemann wrote: > On Tue, Feb 12, 2019 at 09:20:23PM -, Christos Zoulas wrote: > > Well, regardless of what the right permissions of .eh_frame are, > > we could just nuke the code and default to the "new" behavior... > > We can then put back the varasm.c change... > > This of course needs to be evaluated carefully. > > Why would anyone modify the eh frame list at runtime? Or is any other > writable data placed there on purpose? I didn't see any, and if - wouldn't > it better go into a separate section? .eh_frame was historically writeable on MIPS because they f**ked up the relocations: inter-section relocations were not allowed. The "fix" for that was to use indirect pointer encodings and allow ld to optimize the .eh_frame encoding. Joerg
re: CVS commit: src/usr.bin/crunch/crunchgen
> >> The real bug is the reverted varasm.c change. GCC creates the .eh_frame > >> section with the wrong permissions. > > > >yes - putting your varasm.c back fixes the crtbegin.o > >build, but it breaks the libstdc++ one: > > > >In file included from > >/usr/src4/external/gpl3/gcc/dist/libstdc++-v3/src/c++98/pool_allocator.cc:31:0: > >/usr/src4/external/gpl3/gcc/dist/libstdc++-v3/include/ext/pool_allocator.h:210:5: > > error: only zero initializers are allowed in section > > __pool_alloc<_Tp>::_S_force_new; > > ^ > >i'm not sure how to solve this -- we apparently want both > >behaviours with GCC 7. uwe@ suggested an explicit asm() > >or .S file for the crtbegin issue. > > Isn't this used only to deal with "GLIBCXX_FORCE_NEW"? And "NEW" is a > misnomer since this thing started more than 10 years ago, and it still > bites: https://www.zerotier.com/blog/2017-05-05-theleak.shtml > > Well, regardless of what the right permissions of .eh_frame are, > we could just nuke the code and default to the "new" behavior... > We can then put back the varasm.c change... > This of course needs to be evaluated carefully. FWIW, there are several files that have problem with the varasm.c change applied. https://www.netbsd.org/~mrg/gcc7-no-varasm-change-vs-libstdc++.txt all these fail: compile libstdc++-v3/bitmap_allocator.o compile libstdc++-v3/pool_allocator.o compile libstdc++-v3/mt_allocator.o compile libstdc++-v3/locale-inst.o compile libstdc++-v3/string-inst.o compile libstdc++-v3/wlocale-inst.o compile libstdc++-v3/wstring-inst.o christos, can you be more detailed in what exactly you think we can delete or so? it's not just stuff related to the GLIBCXX_FORCE_NEW variable AFAICT. .mrg.
Re: CVS commit: src/usr.bin/crunch/crunchgen
In article <20190213065626.ga22...@mail.duskware.de>, Martin Husemann wrote: >On Tue, Feb 12, 2019 at 09:20:23PM -, Christos Zoulas wrote: >> Well, regardless of what the right permissions of .eh_frame are, >> we could just nuke the code and default to the "new" behavior... >> We can then put back the varasm.c change... >> This of course needs to be evaluated carefully. > >Why would anyone modify the eh frame list at runtime? Or is any other >writable data placed there on purpose? I didn't see any, and if - wouldn't >it better go into a separate section? I understand that, this is why I am saying that removing the code that triggers the bug, we can restore the varasm change that makes the segment readonly... We can then take our time (or have the gcc folks) fix the underlying issue. christos
Re: CVS commit: src/usr.bin/crunch/crunchgen
On Tue, Feb 12, 2019 at 09:20:23PM -, Christos Zoulas wrote: > Well, regardless of what the right permissions of .eh_frame are, > we could just nuke the code and default to the "new" behavior... > We can then put back the varasm.c change... > This of course needs to be evaluated carefully. Why would anyone modify the eh frame list at runtime? Or is any other writable data placed there on purpose? I didn't see any, and if - wouldn't it better go into a separate section? Martin
Re: CVS commit: src/usr.bin/crunch/crunchgen
In article <16690.1550005...@splode.eterna.com.au>, matthew green wrote: >Joerg Sonnenberger writes: >> On Tue, Feb 12, 2019 at 10:16:58AM +, matthew green wrote: >> > Module Name: src >> > Committed By: mrg >> > Date: Tue Feb 12 10:16:58 UTC 2019 >> > >> > Modified Files: >> >src/usr.bin/crunch/crunchgen: crunchgen.c >> > >> > Log Message: >> > hack alert time: >> > >> > on sparc and sparc64, don't remove .eh_frame section. it leads >> > to failure as something is referenced, and objcopy ends up >> > emitting a broken binary that can't be run -- it attempts to >> > load at va=0, beyond having missing referenced data. >> > >> > also, on sparc64 also don't remove .note.netbsd.mcmodel. >> > >> > the former should be revised when we can avoid it. >> >> The real bug is the reverted varasm.c change. GCC creates the .eh_frame >> section with the wrong permissions. > >yes - putting your varasm.c back fixes the crtbegin.o >build, but it breaks the libstdc++ one: > >In file included from >/usr/src4/external/gpl3/gcc/dist/libstdc++-v3/src/c++98/pool_allocator.cc:31:0: >/usr/src4/external/gpl3/gcc/dist/libstdc++-v3/include/ext/pool_allocator.h:210:5: > error: only zero initializers are allowed in section > __pool_alloc<_Tp>::_S_force_new; > ^ >i'm not sure how to solve this -- we apparently want both >behaviours with GCC 7. uwe@ suggested an explicit asm() >or .S file for the crtbegin issue. Isn't this used only to deal with "GLIBCXX_FORCE_NEW"? And "NEW" is a misnomer since this thing started more than 10 years ago, and it still bites: https://www.zerotier.com/blog/2017-05-05-theleak.shtml Well, regardless of what the right permissions of .eh_frame are, we could just nuke the code and default to the "new" behavior... We can then put back the varasm.c change... This of course needs to be evaluated carefully. christos
re: CVS commit: src/usr.bin/crunch/crunchgen
Joerg Sonnenberger writes: > On Tue, Feb 12, 2019 at 10:16:58AM +, matthew green wrote: > > Module Name:src > > Committed By: mrg > > Date: Tue Feb 12 10:16:58 UTC 2019 > > > > Modified Files: > > src/usr.bin/crunch/crunchgen: crunchgen.c > > > > Log Message: > > hack alert time: > > > > on sparc and sparc64, don't remove .eh_frame section. it leads > > to failure as something is referenced, and objcopy ends up > > emitting a broken binary that can't be run -- it attempts to > > load at va=0, beyond having missing referenced data. > > > > also, on sparc64 also don't remove .note.netbsd.mcmodel. > > > > the former should be revised when we can avoid it. > > The real bug is the reverted varasm.c change. GCC creates the .eh_frame > section with the wrong permissions. yes - putting your varasm.c back fixes the crtbegin.o build, but it breaks the libstdc++ one: In file included from /usr/src4/external/gpl3/gcc/dist/libstdc++-v3/src/c++98/pool_allocator.cc:31:0: /usr/src4/external/gpl3/gcc/dist/libstdc++-v3/include/ext/pool_allocator.h:210:5: error: only zero initializers are allowed in section '.bss._ZN9__gnu_cxx12__pool_allocIwE12_S_force_newE' __pool_alloc<_Tp>::_S_force_new; ^ i'm not sure how to solve this -- we apparently want both behaviours with GCC 7. uwe@ suggested an explicit asm() or .S file for the crtbegin issue. .mrg.
Re: CVS commit: src/usr.bin/crunch/crunchgen
On Tue, Feb 12, 2019 at 10:16:58AM +, matthew green wrote: > Module Name: src > Committed By: mrg > Date: Tue Feb 12 10:16:58 UTC 2019 > > Modified Files: > src/usr.bin/crunch/crunchgen: crunchgen.c > > Log Message: > hack alert time: > > on sparc and sparc64, don't remove .eh_frame section. it leads > to failure as something is referenced, and objcopy ends up > emitting a broken binary that can't be run -- it attempts to > load at va=0, beyond having missing referenced data. > > also, on sparc64 also don't remove .note.netbsd.mcmodel. > > the former should be revised when we can avoid it. The real bug is the reverted varasm.c change. GCC creates the .eh_frame section with the wrong permissions. Joerg
