Re: CVS commit: src/usr.bin/make
On Tue, Jan 26, 2021 at 11:39:52PM +0100, Roland Illig wrote: > The code of usr.bin/make gets distributed to a wider audience by Simon's > bmake distribution (http://www.crufty.net/help/sjg/bmake.html), that's > where the requirement of supporting C89 compilers comes from. At the > time I committed this fix, Simon had managed to dig out an old Solaris 9 > installation with GCC, and these few changes were the only ones needed > to let bmake run on that platform. That sounded easy enough to me. Also note that we need bmake during bootstrap of pkgsrc, and besides finding a working compiler it is one of the early things you need to make work on an ancient platform if you try to bring it to new use (I have been there with Solaris 2.6 at one point, but gave up for other reasons - and of course the machine in question now runs NetBSD (again) [the Solaris adventure was a temporary thing anyway and as pkgsrc did not help as a "plug and play" way to get a usable dev/debug environment it was not worth pushing]). But I must admit that the *commit log* of that change sounded way more scary than the actual change is: replace %zu with %u in printf calls would be plain wrong and of course break (either at runtime or if lucky at compile time) many, many platforms. But - (void)fprintf(f, "\"%s\" line %zu: ", fname, lineno); + (void)fprintf(f, "\"%s\" line %u: ", fname, (unsigned)lineno); is ok for portable code and "lineno" referencing (I guess) a makefile. It could have been (long unsigned) and "%lu", but maybe portability to systems where that would make a difference is a bit far stretched. Martin
Re: CVS commit: src/sys/sys
> On Jan 26, 2021, at 6:49 PM, Valery Ushakov wrote: > > It's hardly fair to characterize three people politely inquiring about > that choice and pointing out a more standard way to spell what you > want to express (that is perfectly in rhyme with the preceding table > and is only one character longer too) as "angry hordes objecting". Sorry, should have added the "/s" at the end. Was kind of a long day, and this was a misguided attempt at decompression humor. -- thorpej
Re: CVS commit: src/sys/sys
On Wed, Jan 27, 2021 at 01:00:05 +, Jason R Thorpe wrote: > Module Name: src > Committed By: thorpej > Date: Wed Jan 27 01:00:05 UTC 2021 > > Modified Files: > src/sys/sys: device.h > > Log Message: > Define a macro to terminate the common usage of device_compatible_entry > arrays, in order to placate the angry hordes objecting to use of a GCC > extension. It's hardly fair to characterize three people politely inquiring about that choice and pointing out a more standard way to spell what you want to express (that is perfectly in rhyme with the preceding table and is only one character longer too) as "angry hordes objecting". The point is moot anyway, since anonymous unions themselves only officially appeared in C11. -uwe
Re: CVS commit: src/usr.bin/make
On 26.01.2021 21:04, Christos Zoulas wrote: Yes, and I did not push for it for the exact reasons stated below: There was only a handful of cases and those can be handled with casts or a macro for now. I am questioning though the utility of maintaining compatibility with platforms that do not support C99 20 years later, vs. using %u and casting or using a formatting macro. The support for ancient platforms is not only with regards to "%zu" vs. "%u", in util.c there is even support for platforms that lack getenv(). Apparently nobody cared enough to remove these bits, even though getenv is guaranteed to exist since C89 already. The code of usr.bin/make gets distributed to a wider audience by Simon's bmake distribution (http://www.crufty.net/help/sjg/bmake.html), that's where the requirement of supporting C89 compilers comes from. At the time I committed this fix, Simon had managed to dig out an old Solaris 9 installation with GCC, and these few changes were the only ones needed to let bmake run on that platform. That sounded easy enough to me. Roland
Re: CVS commit: src/usr.bin/make
Yes, and I did not push for it for the exact reasons stated below: There was only a handful of cases and those can be handled with casts or a macro for now. I am questioning though the utility of maintaining compatibility with platforms that do not support C99 20 years later, vs. using %u and casting or using a formatting macro. christos > On Jan 26, 2021, at 2:00 PM, Roland Illig wrote: > > On 26.01.2021 11:19, Rin Okuyama wrote: >> Ping? >> >> I don't think this is correct fix either. >> Can you please revert this commit? > > Sorry, I didn't get the first mail from Christos back in December, > that's why I didn't take any action. > > Why shouldn't the fix I did be correct? I carefully checked the few > places where I added the casts, and none of the numbers that are > involved will ever be more than a billion. > > 1. The number of variables in a .for loop > > 2. The number of items in a .for loop > > 3. A single line of a text file in meta mode. (Only in debug mode.) > > 4. The number of lines in a single makefile. > > 5. The capturing subexpression in a :C variable modifier, which only > ever ranges from 0 to 9. > > 6. The number of words into which a variable value is split. (Only in > debug mode.) > > Given these, why should a simple %u not be appropriate? I don't want to > clutter the code with another preprocessor macro, therefore I'd like to > keep the code the way it is right now. > > I also don't see why this could ever have the slightest chance of > "breaking everyone else". > > For reference and easy viewing, here is the corresponding commit on > GitHub: > https://github.com/NetBSD/src/commit/ff11c7d3497a40c90ec70101ad72612e2f0884b2 > > Roland > >> On 2020/12/15 7:57, Christos Zoulas wrote: >>> In article <20201213212746.3cfc3f...@cvs.netbsd.org>, >>> Roland Illig wrote: -=-=-=-=-=- Module Name:src Committed By:rillig Date:Sun Dec 13 21:27:46 UTC 2020 Modified Files: src/usr.bin/make: for.c meta.c parse.c var.c Log Message: make(1): replace %zu with %u in printf calls This is needed to compile bmake with GCC 2.8.1 on SunOS 5.9. To support ancient systems like this, the whole code of usr.bin/make is supposed to use only ISO C90 features, except for filemon, which is not used on these systems. >>> >>> Please revert! This breaks everyone else. %zu is the format to >>> print size_t. Last I checked SunOS 5.9 has been dead since 2014 >>> and whoever is still using it might as well install a new compiler, >>> or tie the box at the end of a long chain so it can find its true >>> calling. If you really want to support it instead define MAKE_FMT_SIZE_T >>> and conditionalize it properly for "ancient OS", windows, cygwin, >>> mingwin, and regular folks (this does not even handle "ancient os"): >>> >>> https://github.com/file/file/blob/master/src/file.h#L55 >>> >>> I am still against it though... >>> >>> christos >>> signature.asc Description: Message signed with OpenPGP
Re: CVS commit: src/usr.bin/make
On 26.01.2021 11:19, Rin Okuyama wrote: Ping? I don't think this is correct fix either. Can you please revert this commit? Sorry, I didn't get the first mail from Christos back in December, that's why I didn't take any action. Why shouldn't the fix I did be correct? I carefully checked the few places where I added the casts, and none of the numbers that are involved will ever be more than a billion. 1. The number of variables in a .for loop 2. The number of items in a .for loop 3. A single line of a text file in meta mode. (Only in debug mode.) 4. The number of lines in a single makefile. 5. The capturing subexpression in a :C variable modifier, which only ever ranges from 0 to 9. 6. The number of words into which a variable value is split. (Only in debug mode.) Given these, why should a simple %u not be appropriate? I don't want to clutter the code with another preprocessor macro, therefore I'd like to keep the code the way it is right now. I also don't see why this could ever have the slightest chance of "breaking everyone else". For reference and easy viewing, here is the corresponding commit on GitHub: https://github.com/NetBSD/src/commit/ff11c7d3497a40c90ec70101ad72612e2f0884b2 Roland On 2020/12/15 7:57, Christos Zoulas wrote: In article <20201213212746.3cfc3f...@cvs.netbsd.org>, Roland Illig wrote: -=-=-=-=-=- Module Name: src Committed By: rillig Date: Sun Dec 13 21:27:46 UTC 2020 Modified Files: src/usr.bin/make: for.c meta.c parse.c var.c Log Message: make(1): replace %zu with %u in printf calls This is needed to compile bmake with GCC 2.8.1 on SunOS 5.9. To support ancient systems like this, the whole code of usr.bin/make is supposed to use only ISO C90 features, except for filemon, which is not used on these systems. Please revert! This breaks everyone else. %zu is the format to print size_t. Last I checked SunOS 5.9 has been dead since 2014 and whoever is still using it might as well install a new compiler, or tie the box at the end of a long chain so it can find its true calling. If you really want to support it instead define MAKE_FMT_SIZE_T and conditionalize it properly for "ancient OS", windows, cygwin, mingwin, and regular folks (this does not even handle "ancient os"): https://github.com/file/file/blob/master/src/file.h#L55 I am still against it though... christos
Re: CVS commit: src/sys/dev/pci
On Tue, Jan 26, 2021 at 05:51:42PM +0900, Rin Okuyama wrote: > Hi, > This seems not correct for me. Is the attached patch OK with you? Well you spotted a bug indeed int he freeing section. I'll fix and commit it. Thanks for reporting. Reinoud signature.asc Description: PGP signature
Re: CVS commit: src/usr.bin/make
Ping? I don't think this is correct fix either. Can you please revert this commit? Thanks, rin On 2020/12/15 7:57, Christos Zoulas wrote: In article <20201213212746.3cfc3f...@cvs.netbsd.org>, Roland Illig wrote: -=-=-=-=-=- Module Name:src Committed By: rillig Date: Sun Dec 13 21:27:46 UTC 2020 Modified Files: src/usr.bin/make: for.c meta.c parse.c var.c Log Message: make(1): replace %zu with %u in printf calls This is needed to compile bmake with GCC 2.8.1 on SunOS 5.9. To support ancient systems like this, the whole code of usr.bin/make is supposed to use only ISO C90 features, except for filemon, which is not used on these systems. Please revert! This breaks everyone else. %zu is the format to print size_t. Last I checked SunOS 5.9 has been dead since 2014 and whoever is still using it might as well install a new compiler, or tie the box at the end of a long chain so it can find its true calling. If you really want to support it instead define MAKE_FMT_SIZE_T and conditionalize it properly for "ancient OS", windows, cygwin, mingwin, and regular folks (this does not even handle "ancient os"): https://github.com/file/file/blob/master/src/file.h#L55 I am still against it though... christos
Re: CVS commit: src/sys/dev/pci
Hi,
On 2021/01/25 0:33, Reinoud Zandijk wrote:
Module Name:src
Committed By: reinoud
Date: Sun Jan 24 15:33:02 UTC 2021
Modified Files:
src/sys/dev/pci: virtio_pci.c
Log Message:
On error unmap the pci_mapreg_map()d regions using bus_space_unmap() as
suggested by jak@
To generate a diff of this commit:
cvs rdiff -u -r1.22 -r1.23 src/sys/dev/pci/virtio_pci.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
This seems not correct for me. Is the attached patch OK with you?
Thanks,
rin
Index: sys/dev/pci/virtio_pci.c
===
RCS file: /home/netbsd/src/sys/dev/pci/virtio_pci.c,v
retrieving revision 1.25
diff -p -u -r1.25 virtio_pci.c
--- sys/dev/pci/virtio_pci.c24 Jan 2021 15:59:35 - 1.25
+++ sys/dev/pci/virtio_pci.c26 Jan 2021 08:47:23 -
@@ -444,7 +444,7 @@ virtio_pci_attach_10(device_t self, void
bus_size_t bars[NMAPREG] = { 0 };
int bars_idx[NMAPREG] = { 0 };
struct virtio_pci_cap *caps[] = { &common, &isr, &device, ¬ify.cap };
- int i, j = 0, ret = 0;
+ int i, j, ret = 0;
if (virtio_pci_find_cap(psc, VIRTIO_PCI_CAP_COMMON_CFG,
&common, sizeof(common)))
@@ -471,7 +471,7 @@ virtio_pci_attach_10(device_t self, void
bars[bar] = len;
}
- for (i = 0; i < __arraycount(bars); i++) {
+ for (i = j = 0; i < __arraycount(bars); i++) {
int reg;
pcireg_t type;
if (bars[i] == 0)
@@ -550,11 +550,12 @@ virtio_pci_attach_10(device_t self, void
err:
/* undo our pci_mapreg_map()s */
- for (i = 0; i < __arraycount(bars); i++) {
+ for (i = j = 0; i < __arraycount(bars); i++) {
if (bars[i] == 0)
continue;
bus_space_unmap(psc->sc_bars_iot[j], psc->sc_bars_ioh[j],
psc->sc_bars_iosize[j]);
+ j++;
}
return ret;
}
