NREL Radiance re-distribution

radiance-dev
1
2

Ah, so now I figured that this wasn't actually on radiance-dev to begin with...
Mostapha, have you considered joining the list for a while?
And please see my query just below.

Cheers
-schorsch

···

Am 2016-05-12 09:10, schrieb Georg Mischler:

There's no way for this to be the same problem we recently identified.
For one, I don't think Robs gcc is using the MS Universal CRT from
Windows 10 which caused that one. And secondly, the character that gets
eaten here is nowhere near a line ending.

Mostapha, can you send me your current test dataset? I only have the
version with 420 names, which does not seem to cause trouble anymore.
Then I'll check if the SCons build has the same issue, and if so,
walk through it to see what really happens.

Cheers
-schorsch

PS: I'm following radiance-dev, no need for a seperate cc.

Am 2016-05-12 00:39, schrieb Gregory J. Ward:

OK, this seems to happen when a Windows read operation ends halfway
through a "\r\n" sequence, leaving a '\r' at the end of one buffer and
reading a '\n' character at the beginning of the next read() call. It
isn't a bug in my code as far as I'm concerned, as Unix handles this
case just fine. Rather, I think there's a bug in the way Windows
replaces "\r\n" with "\n" so that it ends up replacing "\ns" in this
case with the empty string, effectively lobbing the first character
off the returned buffer.

We can test this by running a couple of read() calls on an input
buffer such that the length of the read splits an EOL sequence. This
may be the source of the occasional dropped characters Schorsch was
seeing in his earlier tests. Microsoft said they will fix this at
some point....

Meanwhile, we could try setting the _O_BINARY flag in the open()
command in wordfile(), assuming we won't run into the ^Z EOF marker
that Schorsch claims is just a childhood trauma of mine and nothing to
fear these days...

If Rob is willing to re-compile the librt.a in src/common using the
attached version of wordfile.c and link it with the debug version of
rcontrib, we can see if it makes any difference to test my hypothesis.

Cheers,
-Greg

FROM: Mostapha Sadeghipour <[email protected]>

SUBJECT: Re: NREL Radiance re-distribution

DATE: May 11, 2016 3:17:59 PM PDT

Thanks Greg! Let me know if there is anything that I can do on my
side to help.
Mostapha

On Wed, May 11, 2016 at 6:09 PM, Gregory J. Ward >>> <[email protected]> wrote:

Hi Mostapha,

Your screen capture came through -- now we're getting somewhere!
The error again happens near the boundary between read() calls, so
it's a matter of figuring out what could be going wrong on Windows
even after the earlier bug fix.

Schorsch has noticed issues with dropped bytes on stdin, but I don't
think we've seen this sort of problem with file input. It could
still have something to do with the conversion of "\r\n" EOL
sequences to "\n" in O_TEXT input files, but I need to think how
this might happen.

More later,
-Greg

FROM: Mostapha Sadeghipour <[email protected]>

DATE: May 11, 2016 2:44:18 PM PDT

Thanks Rob! Dropbox link worked. Maybe I was doing something wrong.

I think we're close. New rcontrib prints out some notes which show
that solar1216 is picked up as 8-letter modifier `olar1216` missing
the starting s. The rest are 9 letter modifiers. That should be why
it never shows up?

I almost never copy images in an email but I hope this one shows up
right. Let me know if it wasn't and I can save and attach it.

Mostapha

--
Georg Mischler -- simulations developer -- schorsch at schorsch com
+schorsch.com+ -- lighting design tools -- http://www.schorsch.com/

3

Hi Schorsch,

You can download the files from this link
<https://dl.dropboxusercontent.com/u/16228160/sunlighthours_round_III.zip>.

For anyone who might be interested here is the original discussion
<https://unmethours.com/question/17201/rcontrib-misses-one-of-the-modifiers-if-number-of-modifiers-between-240-420/>
on
unmethours.

Thanks,
Mostapha

···

On Thu, May 12, 2016 at 4:09 AM, Georg Mischler <[email protected]> wrote:

Ah, so now I figured that this wasn't actually on radiance-dev to begin
with...
Mostapha, have you considered joining the list for a while?
And please see my query just below.

Cheers
-schorsch

Am 2016-05-12 09:10, schrieb Georg Mischler:

There's no way for this to be the same problem we recently identified.
For one, I don't think Robs gcc is using the MS Universal CRT from
Windows 10 which caused that one. And secondly, the character that gets
eaten here is nowhere near a line ending.

Mostapha, can you send me your current test dataset? I only have the
version with 420 names, which does not seem to cause trouble anymore.
Then I'll check if the SCons build has the same issue, and if so,
walk through it to see what really happens.

Cheers
-schorsch

PS: I'm following radiance-dev, no need for a seperate cc.

Am 2016-05-12 00:39, schrieb Gregory J. Ward:

OK, this seems to happen when a Windows read operation ends halfway
through a "\r\n" sequence, leaving a '\r' at the end of one buffer and
reading a '\n' character at the beginning of the next read() call. It
isn't a bug in my code as far as I'm concerned, as Unix handles this
case just fine. Rather, I think there's a bug in the way Windows
replaces "\r\n" with "\n" so that it ends up replacing "\ns" in this
case with the empty string, effectively lobbing the first character
off the returned buffer.

We can test this by running a couple of read() calls on an input
buffer such that the length of the read splits an EOL sequence. This
may be the source of the occasional dropped characters Schorsch was
seeing in his earlier tests. Microsoft said they will fix this at
some point....

Meanwhile, we could try setting the _O_BINARY flag in the open()
command in wordfile(), assuming we won't run into the ^Z EOF marker
that Schorsch claims is just a childhood trauma of mine and nothing to
fear these days...

If Rob is willing to re-compile the librt.a in src/common using the
attached version of wordfile.c and link it with the debug version of
rcontrib, we can see if it makes any difference to test my hypothesis.

Cheers,
-Greg

FROM: Mostapha Sadeghipour <[email protected]>

SUBJECT: Re: NREL Radiance re-distribution

DATE: May 11, 2016 3:17:59 PM PDT

Thanks Greg! Let me know if there is anything that I can do on my

side to help.
Mostapha

On Wed, May 11, 2016 at 6:09 PM, Gregory J. Ward >>>> <[email protected]> wrote:

Hi Mostapha,

Your screen capture came through -- now we're getting somewhere!
The error again happens near the boundary between read() calls, so
it's a matter of figuring out what could be going wrong on Windows
even after the earlier bug fix.

Schorsch has noticed issues with dropped bytes on stdin, but I don't
think we've seen this sort of problem with file input. It could
still have something to do with the conversion of "\r\n" EOL
sequences to "\n" in O_TEXT input files, but I need to think how
this might happen.

More later,
-Greg

FROM: Mostapha Sadeghipour <[email protected]>

DATE: May 11, 2016 2:44:18 PM PDT

Thanks Rob! Dropbox link worked. Maybe I was doing something wrong.

I think we're close. New rcontrib prints out some notes which show
that solar1216 is picked up as 8-letter modifier `olar1216` missing
the starting s. The rest are 9 letter modifiers. That should be why
it never shows up?

I almost never copy images in an email but I hope this one shows up
right. Let me know if it wasn't and I can save and attach it.

Mostapha

--
Georg Mischler -- simulations developer -- schorsch at schorsch com
+schorsch.com+ -- lighting design tools -- http://www.schorsch.com/

4

Well, due to the way wordfile.c backs up over words split by read buffer boundaries, its effective buffer length changes, so it does in fact read this particular line in a way that splits the EOL character. I had to simulate it, but I think I counted correctly.

It will be interesting to see if your build has the same issue....

Cheers,
-Greg

···

From: Georg Mischler <[email protected]>
Subject: Re: [Radiance-dev] NREL Radiance re-distribution
Date: May 12, 2016 12:10:56 AM PDT

There's no way for this to be the same problem we recently identified.
For one, I don't think Robs gcc is using the MS Universal CRT from
Windows 10 which caused that one. And secondly, the character that gets
eaten here is nowhere near a line ending.

Mostapha, can you send me your current test dataset? I only have the
version with 420 names, which does not seem to cause trouble anymore.
Then I'll check if the SCons build has the same issue, and if so,
walk through it to see what really happens.

Cheers
-schorsch

PS: I'm following radiance-dev, no need for a seperate cc.

5

Adding 9-letter modifier 'solar1215'
Adding 8-letter modifier 'olar1216'
Adding 9-letter modifier 'solar1217'

Not really looking forward to the digging...

Cheers
-schorsch

···

Am 2016-05-12 17:49, schrieb Gregory J. Ward:

Well, due to the way wordfile.c backs up over words split by read
buffer boundaries, its effective buffer length changes, so it does in
fact read this particular line in a way that splits the EOL character.
I had to simulate it, but I think I counted correctly.

It will be interesting to see if your build has the same issue....

Cheers,
-Greg

From: Georg Mischler <[email protected]>
Subject: Re: [Radiance-dev] NREL Radiance re-distribution
Date: May 12, 2016 12:10:56 AM PDT

There's no way for this to be the same problem we recently identified.
For one, I don't think Robs gcc is using the MS Universal CRT from
Windows 10 which caused that one. And secondly, the character that gets
eaten here is nowhere near a line ending.

Mostapha, can you send me your current test dataset? I only have the
version with 420 names, which does not seem to cause trouble anymore.
Then I'll check if the SCons build has the same issue, and if so,
walk through it to see what really happens.

Cheers
-schorsch

PS: I'm following radiance-dev, no need for a seperate cc.

_______________________________________________
Radiance-dev mailing list
[email protected]
http://www.radiance-online.org/mailman/listinfo/radiance-dev

--
Georg Mischler -- simulations developer -- schorsch at schorsch com
+schorsch.com+ -- lighting design tools -- http://www.schorsch.com/

6

Should be reproducible with a sequence of read calls on a file with one or two printing characters per line, followed by "\r\n" as usual. If a character gets eaten, then it must be the _O_TEXT processing at fault, somehow.

-Greg

···

From: Georg Mischler <[email protected]>
Subject: Re: [Radiance-dev] NREL Radiance re-distribution
Date: May 12, 2016 12:29:03 PM PDT

Adding 9-letter modifier 'solar1215'
Adding 8-letter modifier 'olar1216'
Adding 9-letter modifier 'solar1217'

Not really looking forward to the digging...

Cheers
-schorsch

Am 2016-05-12 17:49, schrieb Gregory J. Ward:

Well, due to the way wordfile.c backs up over words split by read
buffer boundaries, its effective buffer length changes, so it does in
fact read this particular line in a way that splits the EOL character.
I had to simulate it, but I think I counted correctly.
It will be interesting to see if your build has the same issue....
Cheers,
-Greg

From: Georg Mischler <[email protected]>
Subject: Re: [Radiance-dev] NREL Radiance re-distribution
Date: May 12, 2016 12:10:56 AM PDT
There's no way for this to be the same problem we recently identified.
For one, I don't think Robs gcc is using the MS Universal CRT from
Windows 10 which caused that one. And secondly, the character that gets
eaten here is nowhere near a line ending.
Mostapha, can you send me your current test dataset? I only have the
version with 420 names, which does not seem to cause trouble anymore.
Then I'll check if the SCons build has the same issue, and if so,
walk through it to see what really happens.
Cheers
-schorsch
PS: I'm following radiance-dev, no need for a seperate cc.

_______________________________________________
Radiance-dev mailing list
[email protected]
http://www.radiance-online.org/mailman/listinfo/radiance-dev

--
Georg Mischler -- simulations developer -- schorsch at schorsch com
+schorsch.com+ -- lighting design tools -- http://www.schorsch.com/

_______________________________________________
Radiance-dev mailing list
[email protected]
http://www.radiance-online.org/mailman/listinfo/radiance-dev

7

The problem is that in wordfile(), crem still gets incremented even after
passing the null byte in buf. I must admit that I don't quite understand
the counting logic in there yet, the function looks unnecessarily complicated
to me. Maybe I'll see clearer tomorrow when I'm fully awake again...

Cheers
-schorsch

···

Am 2016-05-13 00:02, schrieb Gregory J. Ward:

Should be reproducible with a sequence of read calls on a file with
one or two printing characters per line, followed by "\r\n" as usual.
If a character gets eaten, then it must be the _O_TEXT processing at
fault, somehow.

-Greg

From: Georg Mischler <[email protected]>
Subject: Re: [Radiance-dev] NREL Radiance re-distribution
Date: May 12, 2016 12:29:03 PM PDT

Adding 9-letter modifier 'solar1215'
Adding 8-letter modifier 'olar1216'
Adding 9-letter modifier 'solar1217'

Not really looking forward to the digging...

Cheers
-schorsch

Am 2016-05-12 17:49, schrieb Gregory J. Ward:

Well, due to the way wordfile.c backs up over words split by read
buffer boundaries, its effective buffer length changes, so it does in
fact read this particular line in a way that splits the EOL character.
I had to simulate it, but I think I counted correctly.
It will be interesting to see if your build has the same issue....
Cheers,
-Greg

From: Georg Mischler <[email protected]>
Subject: Re: [Radiance-dev] NREL Radiance re-distribution
Date: May 12, 2016 12:10:56 AM PDT
There's no way for this to be the same problem we recently identified.
For one, I don't think Robs gcc is using the MS Universal CRT from
Windows 10 which caused that one. And secondly, the character that gets
eaten here is nowhere near a line ending.
Mostapha, can you send me your current test dataset? I only have the
version with 420 names, which does not seem to cause trouble anymore.
Then I'll check if the SCons build has the same issue, and if so,
walk through it to see what really happens.
Cheers
-schorsch
PS: I'm following radiance-dev, no need for a seperate cc.

_______________________________________________
Radiance-dev mailing list
[email protected]
http://www.radiance-online.org/mailman/listinfo/radiance-dev

--
Georg Mischler -- simulations developer -- schorsch at schorsch com
+schorsch.com+ -- lighting design tools -- http://www.schorsch.com/

_______________________________________________
Radiance-dev mailing list
[email protected]
http://www.radiance-online.org/mailman/listinfo/radiance-dev

_______________________________________________
Radiance-dev mailing list
[email protected]
http://www.radiance-online.org/mailman/listinfo/radiance-dev

--
Georg Mischler -- simulations developer -- schorsch at schorsch com
+schorsch.com+ -- lighting design tools -- http://www.schorsch.com/

8

Hmmm... I really seem to be tired.
That null byte is the one we just wrote ourselfes...

But something in the counting is definitively off.
"buf+MAXWLEN-crem" will point to the second character of the next word,
and memmove() probably reads one beyond the buffer.

More tomorrow.

Cheers
-schorsch

···

Am 2016-05-13 00:53, schrieb Georg Mischler:

The problem is that in wordfile(), crem still gets incremented even after
passing the null byte in buf. I must admit that I don't quite understand
the counting logic in there yet, the function looks unnecessarily complicated
to me. Maybe I'll see clearer tomorrow when I'm fully awake again...

Cheers
-schorsch

Am 2016-05-13 00:02, schrieb Gregory J. Ward:

Should be reproducible with a sequence of read calls on a file with
one or two printing characters per line, followed by "\r\n" as usual.
If a character gets eaten, then it must be the _O_TEXT processing at
fault, somehow.

-Greg

From: Georg Mischler <[email protected]>
Subject: Re: [Radiance-dev] NREL Radiance re-distribution
Date: May 12, 2016 12:29:03 PM PDT

Adding 9-letter modifier 'solar1215'
Adding 8-letter modifier 'olar1216'
Adding 9-letter modifier 'solar1217'

Not really looking forward to the digging...

Cheers
-schorsch

Am 2016-05-12 17:49, schrieb Gregory J. Ward:

Well, due to the way wordfile.c backs up over words split by read
buffer boundaries, its effective buffer length changes, so it does in
fact read this particular line in a way that splits the EOL character.
I had to simulate it, but I think I counted correctly.
It will be interesting to see if your build has the same issue....
Cheers,
-Greg

From: Georg Mischler <[email protected]>
Subject: Re: [Radiance-dev] NREL Radiance re-distribution
Date: May 12, 2016 12:10:56 AM PDT
There's no way for this to be the same problem we recently identified.
For one, I don't think Robs gcc is using the MS Universal CRT from
Windows 10 which caused that one. And secondly, the character that gets
eaten here is nowhere near a line ending.
Mostapha, can you send me your current test dataset? I only have the
version with 420 names, which does not seem to cause trouble anymore.
Then I'll check if the SCons build has the same issue, and if so,
walk through it to see what really happens.
Cheers
-schorsch
PS: I'm following radiance-dev, no need for a seperate cc.

_______________________________________________
Radiance-dev mailing list
[email protected]
http://www.radiance-online.org/mailman/listinfo/radiance-dev

--
Georg Mischler -- simulations developer -- schorsch at schorsch com
+schorsch.com+ -- lighting design tools -- http://www.schorsch.com/

_______________________________________________
Radiance-dev mailing list
[email protected]
http://www.radiance-online.org/mailman/listinfo/radiance-dev

_______________________________________________
Radiance-dev mailing list
[email protected]
http://www.radiance-online.org/mailman/listinfo/radiance-dev

--
Georg Mischler -- simulations developer -- schorsch at schorsch com
+schorsch.com+ -- lighting design tools -- http://www.schorsch.com/

9

I agree the code in wordfile.c is a bit over-complex and confusing. Hopefully, it will make more sense to you tomorrow.

Meanwhile, you can try running the following code on the attached files:

#include <stdio.h>
#include "platform.h"

int
main(int argc, char *argv[])
{
  char rdbuf[40];
  int n;

  while ((n = read(0, rdbuf, sizeof(rdbuf))) > 0) {
    char *cp = rdbuf;
    printf("Read %d characters:\n\"", n);
    while (n--)
      fputc(*cp++, stdout);
    fputs("\"\n", stdout);
  }
  return 0;
}

Running "checkread < testfile_unix.txt" gives me the following on Mac OS X:

Read 40 characters:
"123456789
123456789
123456789
123456789
"
Read 20 characters:
"123456789
123456789
"

You should get the same result on Windows using this file. However, using the "testfile_dos.txt" as your input, you might get:

Read 37 characters:
"123456789
123456789
123456789
1234567"
Read 23 characters:
"89
123456789
123456789
"

Using "testfile_dos2.txt", I get:

Read 40 characters:
"12345678
12345678
12345678
12345678
"
Read 20 characters:
"12345678
12345678
"
You should get the same, but with different character counts -- 36 in the first read and 18 in the second as it removes the '\r' characters from the result in the default _O_TEXT mode. Now, try adding an extra character to the beginning of the "testfile_dos2.txt" file, so that the "\r\n" sequence splits the read calls. I'm hoping this will demonstrate the bug, or else I'm still confused about what's going wrong in Windows.

Cheers,
-Greg

testfile_unix.txt (60 Bytes)

testfile_dos.txt (66 Bytes)

testfile_dos2.txt (60 Bytes)

10

Ok, I think I got it figured out.

The code in wordfile() blindly assumed that read() would always return
exactly the number of bytes requested. This is NOT garanteed by the
specification. It's perfectly valid for read() to return less, but
never more data than requested.

Windows is doing something very reasonable here. When the requested
read size would split up a CRNL, then it stops reading one byte earlier.
This ensures that the next read() will be able to correctly convert the
CRNL into a single NL.

The consequence for us is that counting back from the end of MAXWLEN will
give us the wrong position in the data. We need to save the value of n
right after the read(), and use that instead of MAXWLEN in the memmove().
Reading in binary mode is unnecessary.

I've checked a working version into CVS.

Cheers
-schorsch

···

Am 2016-05-13 02:06, schrieb Gregory J. Ward:

I agree the code in wordfile.c is a bit over-complex and confusing.
Hopefully, it will make more sense to you tomorrow.

Meanwhile, you can try running the following code on the attached files:

#include <stdio.h>
#include "platform.h"

int
main(int argc, char *argv[])
{
  char rdbuf[40];
  int n;

  while ((n = read(0, rdbuf, sizeof(rdbuf))) > 0) {
    char *cp = rdbuf;
    printf("Read %d characters:\n\"", n);
    while (n--)
      fputc(*cp++, stdout);
    fputs("\"\n", stdout);
  }
  return 0;
}

Running "checkread < testfile_unix.txt" gives me the following on Mac OS X:

Read 40 characters:
"123456789
123456789
"
Read 20 characters:
"123456789
123456789
"

You should get the same result on Windows using this file. However,
using the "testfile_dos.txt" as your input, you might get:

Read 37 characters:
"123456789
123456789
1234567"
Read 23 characters:
"89
123456789
"

Using "testfile_dos2.txt", I get:

Read 40 characters:
"12345678
12345678
"
Read 20 characters:
"12345678
12345678
"
You should get the same, but with different character counts -- 36 in
the first read and 18 in the second as it removes the '\r' characters
from the result in the default _O_TEXT mode. Now, try adding an extra
character to the beginning of the "testfile_dos2.txt" file, so that
the "\r\n" sequence splits the read calls. I'm hoping this will
demonstrate the bug, or else I'm still confused about what's going
wrong in Windows.

Cheers,
-Greg

From: Georg Mischler <[email protected]>
Subject: Re: [Radiance-dev] NREL Radiance re-distribution
Date: May 12, 2016 4:17:50 PM PDT

Hmmm... I really seem to be tired.
That null byte is the one we just wrote ourselfes...

But something in the counting is definitively off.
"buf+MAXWLEN-crem" will point to the second character of the next word,
and memmove() probably reads one beyond the buffer.

More tomorrow.

Cheers
-schorsch

Am 2016-05-13 00:53, schrieb Georg Mischler:

The problem is that in wordfile(), crem still gets incremented even after
passing the null byte in buf. I must admit that I don't quite understand
the counting logic in there yet, the function looks unnecessarily complicated
to me. Maybe I'll see clearer tomorrow when I'm fully awake again...
Cheers
-schorsch
Am 2016-05-13 00:02, schrieb Gregory J. Ward:

Should be reproducible with a sequence of read calls on a file with
one or two printing characters per line, followed by "\r\n" as usual.
If a character gets eaten, then it must be the _O_TEXT processing at
fault, somehow.
-Greg

_______________________________________________
Radiance-dev mailing list
[email protected]
http://www.radiance-online.org/mailman/listinfo/radiance-dev

--
Georg Mischler -- simulations developer -- schorsch at schorsch com
+schorsch.com+ -- lighting design tools -- http://www.schorsch.com/

11

Ok, I think I got it figured out.

The code in wordfile() blindly assumed that read() would always return
exactly the number of bytes requested. This is NOT garanteed by the
specification. It's perfectly valid for read() to return less, but
never more data than requested.

On Unix a short read from a regular file always means EOF. Read(2) is an operating system function call.

Windows is doing something very reasonable here. When the requested
read size would split up a CRNL, then it stops reading one byte earlier.
This ensures that the next read() will be able to correctly convert the
CRNL into a single NL.

On Windows, apparently, not always. Unix handles such things in the stdio library.

Bah!

Randolph

···

On May 13, 2016, at 2:08 AM, Georg Mischler <[email protected]> wrote:

12

On Unix a short read from a regular file always means EOF. Read(2) is
an operating system function call.

Not necessarily. It could also mean having been interrupted by a signal
(except on systems with BSD semantics). I think we had a similar discussion
just recently here, though I can't find it anymore...

The Linux manpage very succinctly tells us to expect "up to count bytes".
This means that we *must* be prepared for it to deliver a smaller amount.
It usually happens to be the case that unix systems (barring any external
intervention like signals) don't have a reason to read less than was asked for.
But that is just a happy coincidence, and not part of the specification.

Windows is doing something very reasonable here. When the requested
read size would split up a CRNL, then it stops reading one byte earlier.
This ensures that the next read() will be able to correctly convert the
CRNL into a single NL.

On Windows, apparently, not always. Unix handles such things in the
stdio library.

This is managed by the CRT on all systems. The thing on Windows is that
there are many seperate versions of the CRT around, because in the past it
was considered part of the compiler. In the future (starting with Win 10
and VS 2015), the new "universal" CRT is considered part of the OS like
on unix, which will eliminate most inconistencies in the long run.

Cheers
-schorsch

···

Am 2016-05-13 13:36, schrieb Randolph Fritz:

On May 13, 2016, at 2:08 AM, Georg Mischler <[email protected]> >> wrote:

--
Georg Mischler -- simulations developer -- schorsch at schorsch com
+schorsch.com+ -- lighting design tools -- http://www.schorsch.com/

13

Not necessarily. It could also mean having been interrupted by a signal
(except on systems with BSD semantics). I think we had a similar discussion
just recently here, though I can't find it anymore…

You’re right; the Linux man page says:

It is not an error if this number is smaller than the number of bytes requested; this may happen for example because fewer bytes are actually available right now (maybe because we were close to end-of-file, or because we are reading from a pipe, or from a terminal), or because
read() was interrupted by a signal.

Which means this would be an intermittent bug on Unix systems as well. Arrgh.

Randolph

···

On May 13, 2016, at 5:04 AM, Georg Mischler <[email protected]> wrote:

14

Wow -- missed that one! I'm kind of surprised it didn't fail more often... Now, I'm extra curious to see what the test I sent you returns on Windows, since I was assuming the earlier bug (which *was* caused by assuming the requested buffer length was always returned) was caused by short counts in a different context. The bug you just fixed was a remnant of that earlier bug.

-Greg

···

From: Georg Mischler <[email protected]>
Subject: Re: [Radiance-dev] NREL Radiance re-distribution
Date: May 13, 2016 2:08:12 AM PDT

Ok, I think I got it figured out.

The code in wordfile() blindly assumed that read() would always return
exactly the number of bytes requested. This is NOT garanteed by the
specification. It's perfectly valid for read() to return less, but
never more data than requested.

Windows is doing something very reasonable here. When the requested
read size would split up a CRNL, then it stops reading one byte earlier.
This ensures that the next read() will be able to correctly convert the
CRNL into a single NL.

The consequence for us is that counting back from the end of MAXWLEN will
give us the wrong position in the data. We need to save the value of n
right after the read(), and use that instead of MAXWLEN in the memmove().
Reading in binary mode is unnecessary.

I've checked a working version into CVS.

Cheers
-schorsch

Am 2016-05-13 02:06, schrieb Gregory J. Ward:

I agree the code in wordfile.c is a bit over-complex and confusing.
Hopefully, it will make more sense to you tomorrow.
Meanwhile, you can try running the following code on the attached files:
#include <stdio.h>
#include "platform.h"
int
main(int argc, char *argv[])
{
  char rdbuf[40];
  int n;
  while ((n = read(0, rdbuf, sizeof(rdbuf))) > 0) {
    char *cp = rdbuf;
    printf("Read %d characters:\n\"", n);
    while (n--)
      fputc(*cp++, stdout);
    fputs("\"\n", stdout);
  }
  return 0;
}
Running "checkread < testfile_unix.txt" gives me the following on Mac OS X:
Read 40 characters:
"123456789
123456789
123456789
123456789
"
Read 20 characters:
"123456789
123456789
"
You should get the same result on Windows using this file. However,
using the "testfile_dos.txt" as your input, you might get:
Read 37 characters:
"123456789
123456789
123456789
1234567"
Read 23 characters:
"89
123456789
123456789
"
Using "testfile_dos2.txt", I get:
Read 40 characters:
"12345678
12345678
12345678
12345678
"
Read 20 characters:
"12345678
12345678
"
You should get the same, but with different character counts -- 36 in
the first read and 18 in the second as it removes the '\r' characters
from the result in the default _O_TEXT mode. Now, try adding an extra
character to the beginning of the "testfile_dos2.txt" file, so that
the "\r\n" sequence splits the read calls. I'm hoping this will
demonstrate the bug, or else I'm still confused about what's going
wrong in Windows.
Cheers,
-Greg

From: Georg Mischler <[email protected]>
Subject: Re: [Radiance-dev] NREL Radiance re-distribution
Date: May 12, 2016 4:17:50 PM PDT
Hmmm... I really seem to be tired.
That null byte is the one we just wrote ourselfes...
But something in the counting is definitively off.
"buf+MAXWLEN-crem" will point to the second character of the next word,
and memmove() probably reads one beyond the buffer.
More tomorrow.
Cheers
-schorsch
Am 2016-05-13 00:53, schrieb Georg Mischler:

The problem is that in wordfile(), crem still gets incremented even after
passing the null byte in buf. I must admit that I don't quite understand
the counting logic in there yet, the function looks unnecessarily complicated
to me. Maybe I'll see clearer tomorrow when I'm fully awake again...
Cheers
-schorsch
Am 2016-05-13 00:02, schrieb Gregory J. Ward:

Should be reproducible with a sequence of read calls on a file with
one or two printing characters per line, followed by "\r\n" as usual.
If a character gets eaten, then it must be the _O_TEXT processing at
fault, somehow.
-Greg

15

I didn't actually look very closely at your test code before, going for the
real thing first. But now I see what you're trying to figure out.
We couldn't have caught our bug that way, but other than that your prediction
is quite correct:

test.exe < testfile_unix.txt
Read 40 characters:
"123456789
123456789
"
Read 20 characters:
"123456789
123456789
"

test.exe < testfile_dos.txt
Read 37 characters:
"123456789
123456789
1234567"
Read 23 characters:
"89
123456789
"

test.exe < testfile_dos2.txt
Read 36 characters:
"12345678
12345678
"
Read 18 characters:
"12345678
12345678
"

Cheers
-schorsch

···

Am 2016-05-13 18:24, schrieb Gregory J. Ward:

Wow -- missed that one! I'm kind of surprised it didn't fail more
often... Now, I'm extra curious to see what the test I sent you
returns on Windows, since I was assuming the earlier bug (which *was*
caused by assuming the requested buffer length was always returned)
was caused by short counts in a different context. The bug you just
fixed was a remnant of that earlier bug.

-Greg

From: Georg Mischler <[email protected]>
Subject: Re: [Radiance-dev] NREL Radiance re-distribution
Date: May 13, 2016 2:08:12 AM PDT

Ok, I think I got it figured out.

The code in wordfile() blindly assumed that read() would always return
exactly the number of bytes requested. This is NOT garanteed by the
specification. It's perfectly valid for read() to return less, but
never more data than requested.

Windows is doing something very reasonable here. When the requested
read size would split up a CRNL, then it stops reading one byte earlier.
This ensures that the next read() will be able to correctly convert the
CRNL into a single NL.

The consequence for us is that counting back from the end of MAXWLEN will
give us the wrong position in the data. We need to save the value of n
right after the read(), and use that instead of MAXWLEN in the memmove().
Reading in binary mode is unnecessary.

I've checked a working version into CVS.

Cheers
-schorsch

Am 2016-05-13 02:06, schrieb Gregory J. Ward:

I agree the code in wordfile.c is a bit over-complex and confusing.
Hopefully, it will make more sense to you tomorrow.
Meanwhile, you can try running the following code on the attached files:
#include <stdio.h>
#include "platform.h"
int
main(int argc, char *argv[])
{
  char rdbuf[40];
  int n;
  while ((n = read(0, rdbuf, sizeof(rdbuf))) > 0) {
    char *cp = rdbuf;
    printf("Read %d characters:\n\"", n);
    while (n--)
      fputc(*cp++, stdout);
    fputs("\"\n", stdout);
  }
  return 0;
}
Running "checkread < testfile_unix.txt" gives me the following on Mac OS X:
Read 40 characters:
"123456789
123456789
"
Read 20 characters:
"123456789
123456789
"
You should get the same result on Windows using this file. However,
using the "testfile_dos.txt" as your input, you might get:
Read 37 characters:
"123456789
123456789
1234567"
Read 23 characters:
"89
123456789
"
Using "testfile_dos2.txt", I get:
Read 40 characters:
"12345678
12345678
"
Read 20 characters:
"12345678
12345678
"
You should get the same, but with different character counts -- 36 in
the first read and 18 in the second as it removes the '\r' characters
from the result in the default _O_TEXT mode. Now, try adding an extra
character to the beginning of the "testfile_dos2.txt" file, so that
the "\r\n" sequence splits the read calls. I'm hoping this will
demonstrate the bug, or else I'm still confused about what's going
wrong in Windows.
Cheers,
-Greg

From: Georg Mischler <[email protected]>
Subject: Re: [Radiance-dev] NREL Radiance re-distribution
Date: May 12, 2016 4:17:50 PM PDT
Hmmm... I really seem to be tired.
That null byte is the one we just wrote ourselfes...
But something in the counting is definitively off.
"buf+MAXWLEN-crem" will point to the second character of the next word,
and memmove() probably reads one beyond the buffer.
More tomorrow.
Cheers
-schorsch
Am 2016-05-13 00:53, schrieb Georg Mischler:

The problem is that in wordfile(), crem still gets incremented even after
passing the null byte in buf. I must admit that I don't quite understand
the counting logic in there yet, the function looks unnecessarily complicated
to me. Maybe I'll see clearer tomorrow when I'm fully awake again...
Cheers
-schorsch
Am 2016-05-13 00:02, schrieb Gregory J. Ward:

Should be reproducible with a sequence of read calls on a file with
one or two printing characters per line, followed by "\r\n" as usual.
If a character gets eaten, then it must be the _O_TEXT processing at
fault, somehow.
-Greg

_______________________________________________
Radiance-dev mailing list
[email protected]
http://www.radiance-online.org/mailman/listinfo/radiance-dev

--
Georg Mischler -- simulations developer -- schorsch at schorsch com
+schorsch.com+ -- lighting design tools -- http://www.schorsch.com/

16

Thanks, Schorsch,

Would you mind adding the final tests, where you add a character and remove a character from the beginning of testfile_dos2.txt to see what happens? I should have explained that better in my post.

-Greg

···

From: Georg Mischler <[email protected]>
Date: May 13, 2016 10:28:31 AM PDT

I didn't actually look very closely at your test code before, going for the
real thing first. But now I see what you're trying to figure out.
We couldn't have caught our bug that way, but other than that your prediction
is quite correct:

test.exe < testfile_unix.txt
Read 40 characters:
"123456789
123456789
123456789
123456789
"
Read 20 characters:
"123456789
123456789
"

test.exe < testfile_dos.txt
Read 37 characters:
"123456789
123456789
123456789
1234567"
Read 23 characters:
"89
123456789
123456789
"

test.exe < testfile_dos2.txt
Read 36 characters:
"12345678
12345678
12345678
12345678
"
Read 18 characters:
"12345678
12345678
"

Cheers
-schorsch

17

No problem.

### original file
test.exe < testfile_dos2.txt
Read 36 characters:
"12345678
12345678
"
Read 18 characters:
"12345678
12345678
"

### insert a character
test.exe < testfile_dos2.txt
Read 36 characters:
"a12345678
12345678
12345678"
Read 19 characters:
"
12345678
"

### remove first character
test.exe < testfile_dos2.txt
Read 36 characters:
"2345678
12345678
1"
Read 17 characters:
"2345678
12345678
"

···

Am 2016-05-13 20:58, schrieb Gregory J. Ward:

Thanks, Schorsch,

Would you mind adding the final tests, where you add a character and
remove a character from the beginning of testfile_dos2.txt to see what
happens? I should have explained that better in my post.

-Greg

From: Georg Mischler <[email protected]>
Date: May 13, 2016 10:28:31 AM PDT

I didn't actually look very closely at your test code before, going for the
real thing first. But now I see what you're trying to figure out.
We couldn't have caught our bug that way, but other than that your prediction
is quite correct:

test.exe < testfile_unix.txt
Read 40 characters:
"123456789
123456789
"
Read 20 characters:
"123456789
123456789
"

test.exe < testfile_dos.txt
Read 37 characters:
"123456789
123456789
1234567"
Read 23 characters:
"89
123456789
"

test.exe < testfile_dos2.txt
Read 36 characters:
"12345678
12345678
"
Read 18 characters:
"12345678
12345678
"

Cheers
-schorsch

_______________________________________________
Radiance-dev mailing list
[email protected]
http://www.radiance-online.org/mailman/listinfo/radiance-dev

--
Georg Mischler -- simulations developer -- schorsch at schorsch com
+schorsch.com+ -- lighting design tools -- http://www.schorsch.com/

18

Cool -- thanks! This demonstrates the behavior you pointed out where it backs over the '\r' of a "\r\n" sequence in the input, providing it for decode in the next buffer read. As you say, this makes sense.

Cheers,
-Greg

···

From: Georg Mischler <[email protected]>
Subject: Re: [Radiance-dev] NREL Radiance re-distribution
Date: May 13, 2016 12:33:59 PM PDT

No problem.

### original file
test.exe < testfile_dos2.txt
Read 36 characters:
"12345678
12345678
12345678
12345678
"
Read 18 characters:
"12345678
12345678
"

### insert a character
test.exe < testfile_dos2.txt
Read 36 characters:
"a12345678
12345678
12345678
12345678"
Read 19 characters:
"
12345678
12345678
"

### remove first character
test.exe < testfile_dos2.txt
Read 36 characters:
"2345678
12345678
12345678
12345678
1"
Read 17 characters:
"2345678
12345678
"

Am 2016-05-13 20:58, schrieb Gregory J. Ward:

Thanks, Schorsch,
Would you mind adding the final tests, where you add a character and
remove a character from the beginning of testfile_dos2.txt to see what
happens? I should have explained that better in my post.
-Greg

From: Georg Mischler <[email protected]>
Date: May 13, 2016 10:28:31 AM PDT
I didn't actually look very closely at your test code before, going for the
real thing first. But now I see what you're trying to figure out.
We couldn't have caught our bug that way, but other than that your prediction
is quite correct:
test.exe < testfile_unix.txt
Read 40 characters:
"123456789
123456789
123456789
123456789
"
Read 20 characters:
"123456789
123456789
"
test.exe < testfile_dos.txt
Read 37 characters:
"123456789
123456789
123456789
1234567"
Read 23 characters:
"89
123456789
123456789
"
test.exe < testfile_dos2.txt
Read 36 characters:
"12345678
12345678
12345678
12345678
"
Read 18 characters:
"12345678
12345678
"
Cheers
-schorsch