[PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[Feng Gao]

[-- Attachment #1: Type: text/plain, Size: 313 bytes --]

Hi all,

Both of "_IO_UNBUFFERED" and "_IO_LINE_BUF"  are the bit flags, but I
find there are some codes looks like "_IO_LINE_BUF+_IO_UNBUFFERED",
while some codes are "_IO_LINE_BUF|_IO_UNBUFFERED".

I think the former is not good, even though the final result is same.

The attachment is my patch .

Thanks
Feng

[-- Attachment #2: file_flags.diff --]
[-- Type: text/plain, Size: 2763 bytes --]

diff --git a/libio/fileops.c b/libio/fileops.c
index 7c7fef1..93d0d94 100644
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -541,7 +541,7 @@ new_do_write (fp, data, to_do)
   _IO_setg (fp, fp->_IO_buf_base, fp->_IO_buf_base, fp->_IO_buf_base);
   fp->_IO_write_base = fp->_IO_write_ptr = fp->_IO_buf_base;
   fp->_IO_write_end = (fp->_mode <= 0
-		       && (fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+		       && (fp->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
 		       ? fp->_IO_buf_base : fp->_IO_buf_end);
   return count;
 }
@@ -881,7 +881,7 @@ _IO_new_file_overflow (f, ch)
       f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
 
       f->_flags |= _IO_CURRENTLY_PUTTING;
-      if (f->_mode <= 0 && f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+      if (f->_mode <= 0 && f->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
 	f->_IO_write_end = f->_IO_write_ptr;
     }
   if (ch == EOF)
diff --git a/libio/oldfileops.c b/libio/oldfileops.c
index c68ca6a..343875a 100644
--- a/libio/oldfileops.c
+++ b/libio/oldfileops.c
@@ -313,7 +313,7 @@ old_do_write (fp, data, to_do)
     fp->_cur_column = _IO_adjust_column (fp->_cur_column - 1, data, count) + 1;
   _IO_setg (fp, fp->_IO_buf_base, fp->_IO_buf_base, fp->_IO_buf_base);
   fp->_IO_write_base = fp->_IO_write_ptr = fp->_IO_buf_base;
-  fp->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+  fp->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
 		       ? fp->_IO_buf_base : fp->_IO_buf_end);
   return count;
 }
@@ -418,7 +418,7 @@ _IO_old_file_overflow (f, ch)
       f->_IO_write_end = f->_IO_buf_end;
       f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
 
-      if (f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+      if (f->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
 	f->_IO_write_end = f->_IO_write_ptr;
       f->_flags |= _IO_CURRENTLY_PUTTING;
     }
diff --git a/libio/wfileops.c b/libio/wfileops.c
index 3f628bf..14332ac 100644
--- a/libio/wfileops.c
+++ b/libio/wfileops.c
@@ -106,7 +106,7 @@ _IO_wdo_write (fp, data, to_do)
 	     fp->_wide_data->_IO_buf_base);
   fp->_wide_data->_IO_write_base = fp->_wide_data->_IO_write_ptr
     = fp->_wide_data->_IO_buf_base;
-  fp->_wide_data->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+  fp->_wide_data->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
 				   ? fp->_wide_data->_IO_buf_base
 				   : fp->_wide_data->_IO_buf_end);
 
@@ -465,7 +465,7 @@ _IO_wfile_overflow (f, wch)
       f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
 
       f->_flags |= _IO_CURRENTLY_PUTTING;
-      if (f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+      if (f->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
 	f->_wide_data->_IO_write_end = f->_wide_data->_IO_write_ptr;
     }
   if (wch == WEOF)

Re: [PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[Siddhesh Poyarekar]

Please describe the testing you have done to verify that your patch is
correct.  In general for such monotonous changes, it is sufficient to
verify that there is no change in the generated code or any change is
expected and contained.  Also, run the testsuite to verify that no
additional errors are introduced.

Add a space between the operator; I know this was wrong before, but it
should be corrected with this patch.  That is,

    _IO_LINE_BUF|_IO_UNBUFFERED

should be

    _IO_LINE_BUF | _IO_UNBUFFERED

Your patch also needs a ChangeLog entry.  The patch is probably
trivial enough that it does not need a copyright assignment.

Please post the patch with the suggested change, a ChangeLog entry and
a description of how you tested the patch.

Thanks,
Siddhesh

On Tue, Jun 30, 2015 at 07:23:57AM +0800, Feng Gao wrote:
> Hi all,
> 
> Both of "_IO_UNBUFFERED" and "_IO_LINE_BUF"  are the bit flags, but I
> find there are some codes looks like "_IO_LINE_BUF+_IO_UNBUFFERED",
> while some codes are "_IO_LINE_BUF|_IO_UNBUFFERED".
> 
> I think the former is not good, even though the final result is same.
> 
> The attachment is my patch .
> 
> Thanks
> Feng

> diff --git a/libio/fileops.c b/libio/fileops.c
> index 7c7fef1..93d0d94 100644
> --- a/libio/fileops.c
> +++ b/libio/fileops.c
> @@ -541,7 +541,7 @@ new_do_write (fp, data, to_do)
>    _IO_setg (fp, fp->_IO_buf_base, fp->_IO_buf_base, fp->_IO_buf_base);
>    fp->_IO_write_base = fp->_IO_write_ptr = fp->_IO_buf_base;
>    fp->_IO_write_end = (fp->_mode <= 0
> -		       && (fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
> +		       && (fp->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>  		       ? fp->_IO_buf_base : fp->_IO_buf_end);
>    return count;
>  }
> @@ -881,7 +881,7 @@ _IO_new_file_overflow (f, ch)
>        f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
>  
>        f->_flags |= _IO_CURRENTLY_PUTTING;
> -      if (f->_mode <= 0 && f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
> +      if (f->_mode <= 0 && f->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>  	f->_IO_write_end = f->_IO_write_ptr;
>      }
>    if (ch == EOF)
> diff --git a/libio/oldfileops.c b/libio/oldfileops.c
> index c68ca6a..343875a 100644
> --- a/libio/oldfileops.c
> +++ b/libio/oldfileops.c
> @@ -313,7 +313,7 @@ old_do_write (fp, data, to_do)
>      fp->_cur_column = _IO_adjust_column (fp->_cur_column - 1, data, count) + 1;
>    _IO_setg (fp, fp->_IO_buf_base, fp->_IO_buf_base, fp->_IO_buf_base);
>    fp->_IO_write_base = fp->_IO_write_ptr = fp->_IO_buf_base;
> -  fp->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
> +  fp->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>  		       ? fp->_IO_buf_base : fp->_IO_buf_end);
>    return count;
>  }
> @@ -418,7 +418,7 @@ _IO_old_file_overflow (f, ch)
>        f->_IO_write_end = f->_IO_buf_end;
>        f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
>  
> -      if (f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
> +      if (f->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>  	f->_IO_write_end = f->_IO_write_ptr;
>        f->_flags |= _IO_CURRENTLY_PUTTING;
>      }
> diff --git a/libio/wfileops.c b/libio/wfileops.c
> index 3f628bf..14332ac 100644
> --- a/libio/wfileops.c
> +++ b/libio/wfileops.c
> @@ -106,7 +106,7 @@ _IO_wdo_write (fp, data, to_do)
>  	     fp->_wide_data->_IO_buf_base);
>    fp->_wide_data->_IO_write_base = fp->_wide_data->_IO_write_ptr
>      = fp->_wide_data->_IO_buf_base;
> -  fp->_wide_data->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
> +  fp->_wide_data->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>  				   ? fp->_wide_data->_IO_buf_base
>  				   : fp->_wide_data->_IO_buf_end);
>  
> @@ -465,7 +465,7 @@ _IO_wfile_overflow (f, wch)
>        f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
>  
>        f->_flags |= _IO_CURRENTLY_PUTTING;
> -      if (f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
> +      if (f->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>  	f->_wide_data->_IO_write_end = f->_wide_data->_IO_write_ptr;
>      }
>    if (wch == WEOF)

Re: [PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[Feng Gao]

[-- Attachment #1: Type: text/plain, Size: 4700 bytes --]

Hi Siddhesh,

Thanks your response.
The attachment is the latest change according to your suggestion
including the ChangeLog change.

About the tests, I did the following cases:
1. Write test codes to check if the (_IO_LINE_BUF|_IO_UNBUFFERED)
equals  (_IO_LINE_BUF+_IO_UNBUFFERED);
2. Update the glibc to check if it works like before.

On Tue, Jul 7, 2015 at 3:31 PM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
> Please describe the testing you have done to verify that your patch is
> correct.  In general for such monotonous changes, it is sufficient to
> verify that there is no change in the generated code or any change is
> expected and contained.  Also, run the testsuite to verify that no
> additional errors are introduced.
>
> Add a space between the operator; I know this was wrong before, but it
> should be corrected with this patch.  That is,
>
>     _IO_LINE_BUF|_IO_UNBUFFERED
>
> should be
>
>     _IO_LINE_BUF | _IO_UNBUFFERED
>
> Your patch also needs a ChangeLog entry.  The patch is probably
> trivial enough that it does not need a copyright assignment.
>
> Please post the patch with the suggested change, a ChangeLog entry and
> a description of how you tested the patch.
>
> Thanks,
> Siddhesh
>
> On Tue, Jun 30, 2015 at 07:23:57AM +0800, Feng Gao wrote:
>> Hi all,
>>
>> Both of "_IO_UNBUFFERED" and "_IO_LINE_BUF"  are the bit flags, but I
>> find there are some codes looks like "_IO_LINE_BUF+_IO_UNBUFFERED",
>> while some codes are "_IO_LINE_BUF|_IO_UNBUFFERED".
>>
>> I think the former is not good, even though the final result is same.
>>
>> The attachment is my patch .
>>
>> Thanks
>> Feng
>
>> diff --git a/libio/fileops.c b/libio/fileops.c
>> index 7c7fef1..93d0d94 100644
>> --- a/libio/fileops.c
>> +++ b/libio/fileops.c
>> @@ -541,7 +541,7 @@ new_do_write (fp, data, to_do)
>>    _IO_setg (fp, fp->_IO_buf_base, fp->_IO_buf_base, fp->_IO_buf_base);
>>    fp->_IO_write_base = fp->_IO_write_ptr = fp->_IO_buf_base;
>>    fp->_IO_write_end = (fp->_mode <= 0
>> -                    && (fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
>> +                    && (fp->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>>                      ? fp->_IO_buf_base : fp->_IO_buf_end);
>>    return count;
>>  }
>> @@ -881,7 +881,7 @@ _IO_new_file_overflow (f, ch)
>>        f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
>>
>>        f->_flags |= _IO_CURRENTLY_PUTTING;
>> -      if (f->_mode <= 0 && f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
>> +      if (f->_mode <= 0 && f->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>>       f->_IO_write_end = f->_IO_write_ptr;
>>      }
>>    if (ch == EOF)
>> diff --git a/libio/oldfileops.c b/libio/oldfileops.c
>> index c68ca6a..343875a 100644
>> --- a/libio/oldfileops.c
>> +++ b/libio/oldfileops.c
>> @@ -313,7 +313,7 @@ old_do_write (fp, data, to_do)
>>      fp->_cur_column = _IO_adjust_column (fp->_cur_column - 1, data, count) + 1;
>>    _IO_setg (fp, fp->_IO_buf_base, fp->_IO_buf_base, fp->_IO_buf_base);
>>    fp->_IO_write_base = fp->_IO_write_ptr = fp->_IO_buf_base;
>> -  fp->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
>> +  fp->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>>                      ? fp->_IO_buf_base : fp->_IO_buf_end);
>>    return count;
>>  }
>> @@ -418,7 +418,7 @@ _IO_old_file_overflow (f, ch)
>>        f->_IO_write_end = f->_IO_buf_end;
>>        f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
>>
>> -      if (f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
>> +      if (f->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>>       f->_IO_write_end = f->_IO_write_ptr;
>>        f->_flags |= _IO_CURRENTLY_PUTTING;
>>      }
>> diff --git a/libio/wfileops.c b/libio/wfileops.c
>> index 3f628bf..14332ac 100644
>> --- a/libio/wfileops.c
>> +++ b/libio/wfileops.c
>> @@ -106,7 +106,7 @@ _IO_wdo_write (fp, data, to_do)
>>            fp->_wide_data->_IO_buf_base);
>>    fp->_wide_data->_IO_write_base = fp->_wide_data->_IO_write_ptr
>>      = fp->_wide_data->_IO_buf_base;
>> -  fp->_wide_data->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
>> +  fp->_wide_data->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>>                                  ? fp->_wide_data->_IO_buf_base
>>                                  : fp->_wide_data->_IO_buf_end);
>>
>> @@ -465,7 +465,7 @@ _IO_wfile_overflow (f, wch)
>>        f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
>>
>>        f->_flags |= _IO_CURRENTLY_PUTTING;
>> -      if (f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
>> +      if (f->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>>       f->_wide_data->_IO_write_end = f->_wide_data->_IO_write_ptr;
>>      }
>>    if (wch == WEOF)
>

[-- Attachment #2: file_flags.diff --]
[-- Type: text/plain, Size: 3580 bytes --]

diff --git a/ChangeLog b/ChangeLog
index ff64fdc..dcd1058 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+015-07-07  Feng Gao  <gfree.wind@gmail.com>
+	* libio/fileops.c: Use "|" instead of "+" when combine _IO_LINE_BUF
+	and _IO_UNBUFFERED
+	* libio/oldfileops.c: Likewise
+	* libio/wfileops.c: Likewise
+
 2015-07-07  Stefan Liebler  <stli@linux.vnet.ibm.com>
 
 	[BZ #18508]
diff --git a/libio/fileops.c b/libio/fileops.c
index 9668024..cbcd6f5 100644
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -521,7 +521,7 @@ new_do_write (_IO_FILE *fp, const char *data, _IO_size_t to_do)
   _IO_setg (fp, fp->_IO_buf_base, fp->_IO_buf_base, fp->_IO_buf_base);
   fp->_IO_write_base = fp->_IO_write_ptr = fp->_IO_buf_base;
   fp->_IO_write_end = (fp->_mode <= 0
-		       && (fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+		       && (fp->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
 		       ? fp->_IO_buf_base : fp->_IO_buf_end);
   return count;
 }
@@ -844,7 +844,7 @@ _IO_new_file_overflow (_IO_FILE *f, int ch)
       f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
 
       f->_flags |= _IO_CURRENTLY_PUTTING;
-      if (f->_mode <= 0 && f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+      if (f->_mode <= 0 && f->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
 	f->_IO_write_end = f->_IO_write_ptr;
     }
   if (ch == EOF)
diff --git a/libio/oldfileops.c b/libio/oldfileops.c
index 84939e3..54789b2 100644
--- a/libio/oldfileops.c
+++ b/libio/oldfileops.c
@@ -313,7 +313,7 @@ old_do_write (fp, data, to_do)
     fp->_cur_column = _IO_adjust_column (fp->_cur_column - 1, data, count) + 1;
   _IO_setg (fp, fp->_IO_buf_base, fp->_IO_buf_base, fp->_IO_buf_base);
   fp->_IO_write_base = fp->_IO_write_ptr = fp->_IO_buf_base;
-  fp->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+  fp->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
 		       ? fp->_IO_buf_base : fp->_IO_buf_end);
   return count;
 }
@@ -418,7 +418,7 @@ _IO_old_file_overflow (f, ch)
       f->_IO_write_end = f->_IO_buf_end;
       f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
 
-      if (f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+      if (f->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
 	f->_IO_write_end = f->_IO_write_ptr;
       f->_flags |= _IO_CURRENTLY_PUTTING;
     }
diff --git a/libio/wfileops.c b/libio/wfileops.c
index 73d7709..fd51f96 100644
--- a/libio/wfileops.c
+++ b/libio/wfileops.c
@@ -118,7 +118,7 @@ _IO_wdo_write (_IO_FILE *fp, const wchar_t *data, _IO_size_t to_do)
 	     fp->_wide_data->_IO_buf_base);
   fp->_wide_data->_IO_write_base = fp->_wide_data->_IO_write_ptr
     = fp->_wide_data->_IO_buf_base;
-  fp->_wide_data->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+  fp->_wide_data->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
 				   ? fp->_wide_data->_IO_buf_base
 				   : fp->_wide_data->_IO_buf_end);
 
@@ -216,7 +216,7 @@ _IO_wfile_underflow (_IO_FILE *fp)
 
   /* Flush all line buffered files before reading. */
   /* FIXME This can/should be moved to genops ?? */
-  if (fp->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
+  if (fp->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
     {
 #if 0
       _IO_flush_all_linebuffered ();
@@ -477,7 +477,7 @@ _IO_wfile_overflow (_IO_FILE *f, wint_t wch)
       f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
 
       f->_flags |= _IO_CURRENTLY_PUTTING;
-      if (f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+      if (f->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
 	f->_wide_data->_IO_write_end = f->_wide_data->_IO_write_ptr;
     }
   if (wch == WEOF)

Re: [PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[Siddhesh Poyarekar]

On Wed, Jul 08, 2015 at 12:02:38AM +0800, Feng Gao wrote:
> About the tests, I did the following cases:
> 1. Write test codes to check if the (_IO_LINE_BUF|_IO_UNBUFFERED)
> equals  (_IO_LINE_BUF+_IO_UNBUFFERED);
> 2. Update the glibc to check if it works like before.

Thanks, it looks like you missed fixing spacing in the last instance
(quoted below).  Also, run 'make check' before and after the patch to
make sure that there are no regressions due to this change.  Updating
glibc is a brave thing to do - if it breaks, your box is a brick that
only a rescue disk can get back :)

Please post an updated patch and also let me know the results of your
test.

Thanks,
Siddhesh

> @@ -477,7 +477,7 @@ _IO_wfile_overflow (_IO_FILE *f, wint_t wch)
>        f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
>  
>        f->_flags |= _IO_CURRENTLY_PUTTING;
> -      if (f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
> +      if (f->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>  	f->_wide_data->_IO_write_end = f->_wide_data->_IO_write_ptr;
>      }
>    if (wch == WEOF)

Re: [PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[Feng Gao]

[-- Attachment #1: Type: text/plain, Size: 3487 bytes --]

Hi Siddhesh,

Firstly, sorry about that I forget fixing the last space issue. Maybe
I was very tried when it was very late in the night.

Now the attachment "file_flags.diff" is the latest change which fix
the issue you mentioned.
The "test_flag.c" is the test codes to check the flag value.
The "test_buf.c" is the test codes to test if the setbuf of libc works well.

The following are the tests I did:
1. Run the test_flag.c on 32-bits and 64-bits platform, the output is "Equal".
    It means (_IO_UNBUFFERED | _IO_LINE_BUF) has the same value of
(_IO_UNBUFFERED + _IO_LINE_BUF);
2. After update the glibc, gdb the test_buf.c to check if the setbuf
works well like before.
    Because i am using the glibc-2.19 on my linux, so I apply my patch
to glibc-2.19, build and replace the original one. ( I am afraid my
computer become brick:))
    Then gdb the binary of test_buf.c to check if the setbuf works
well like before.
Breakpoint 1, main () at test_buf.c:23
23      {
(gdb) n
26              printf("Before no-buffer.");
(gdb)
27              printf("Output now.\n");
(gdb)
Before no-buffer.Output now.
29              setbuf(stdout, NULL);
(gdb)
30              printf("No buffer now.");
(gdb)
No buffer now.32                setvbuf(stdout, buffer, _IOLBF, sizeof(buffer));
(gdb)
33              printf("Restore linebuf.");
(gdb)
34              printf("Output now.\n");
(gdb)
Restore linebuf.Output now.
36              return 0;

The output is the right behavior we expect.

3. I doesn't execute the "make check". Because it will hang without my
patch. So I could not do "make check" with my patch.
I don't know if there already is one bug in the current glibc codes.

It stop the following step for about 2 hours, so I have to cancel it.

        scripts/evaluate-test.sh c++-types-check $? false false >
/home/fgao/works/my_git_codes/glibc-build/c++-types-check.test-result
AWK='gawk' scripts/check-local-headers.sh \
          "/usr/include" "/home/fgao/works/my_git_codes/glibc-build/"
> /home/fgao/works/my_git_codes/glibc-build/check-local-headers.out; \
        scripts/evaluate-test.sh check-local-headers $? false false >
/home/fgao/works/my_git_codes/glibc-build/check-local-headers.test-result

That's why I did not execute the "make check" with my change.



















On Wed, Jul 8, 2015 at 12:28 AM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
> On Wed, Jul 08, 2015 at 12:02:38AM +0800, Feng Gao wrote:
>> About the tests, I did the following cases:
>> 1. Write test codes to check if the (_IO_LINE_BUF|_IO_UNBUFFERED)
>> equals  (_IO_LINE_BUF+_IO_UNBUFFERED);
>> 2. Update the glibc to check if it works like before.
>
> Thanks, it looks like you missed fixing spacing in the last instance
> (quoted below).  Also, run 'make check' before and after the patch to
> make sure that there are no regressions due to this change.  Updating
> glibc is a brave thing to do - if it breaks, your box is a brick that
> only a rescue disk can get back :)
>
> Please post an updated patch and also let me know the results of your
> test.
>
> Thanks,
> Siddhesh
>
>> @@ -477,7 +477,7 @@ _IO_wfile_overflow (_IO_FILE *f, wint_t wch)
>>        f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
>>
>>        f->_flags |= _IO_CURRENTLY_PUTTING;
>> -      if (f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
>> +      if (f->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>>       f->_wide_data->_IO_write_end = f->_wide_data->_IO_write_ptr;
>>      }
>>    if (wch == WEOF)
>

[-- Attachment #2: file_flags.diff --]
[-- Type: text/plain, Size: 3602 bytes --]

diff --git a/ChangeLog b/ChangeLog
index 2ccf739..2d94135 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+015-07-07  Feng Gao  <gfree.wind@gmail.com>
+	* libio/fileops.c: Use "|" instead of "+" when combine _IO_LINE_BUF
+	and _IO_UNBUFFERED
+	* libio/oldfileops.c: Likewise
+	* libio/wfileops.c: Likewise
+
 2015-07-07  Pavel Kopyl  <p.kopyl@samsung.com>
 	    Mikhail Ilin  <m.ilin@samsung.com>
 
diff --git a/libio/fileops.c b/libio/fileops.c
index 9668024..cbcd6f5 100644
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -521,7 +521,7 @@ new_do_write (_IO_FILE *fp, const char *data, _IO_size_t to_do)
   _IO_setg (fp, fp->_IO_buf_base, fp->_IO_buf_base, fp->_IO_buf_base);
   fp->_IO_write_base = fp->_IO_write_ptr = fp->_IO_buf_base;
   fp->_IO_write_end = (fp->_mode <= 0
-		       && (fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+		       && (fp->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
 		       ? fp->_IO_buf_base : fp->_IO_buf_end);
   return count;
 }
@@ -844,7 +844,7 @@ _IO_new_file_overflow (_IO_FILE *f, int ch)
       f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
 
       f->_flags |= _IO_CURRENTLY_PUTTING;
-      if (f->_mode <= 0 && f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+      if (f->_mode <= 0 && f->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
 	f->_IO_write_end = f->_IO_write_ptr;
     }
   if (ch == EOF)
diff --git a/libio/oldfileops.c b/libio/oldfileops.c
index 84939e3..54789b2 100644
--- a/libio/oldfileops.c
+++ b/libio/oldfileops.c
@@ -313,7 +313,7 @@ old_do_write (fp, data, to_do)
     fp->_cur_column = _IO_adjust_column (fp->_cur_column - 1, data, count) + 1;
   _IO_setg (fp, fp->_IO_buf_base, fp->_IO_buf_base, fp->_IO_buf_base);
   fp->_IO_write_base = fp->_IO_write_ptr = fp->_IO_buf_base;
-  fp->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+  fp->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
 		       ? fp->_IO_buf_base : fp->_IO_buf_end);
   return count;
 }
@@ -418,7 +418,7 @@ _IO_old_file_overflow (f, ch)
       f->_IO_write_end = f->_IO_buf_end;
       f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
 
-      if (f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+      if (f->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
 	f->_IO_write_end = f->_IO_write_ptr;
       f->_flags |= _IO_CURRENTLY_PUTTING;
     }
diff --git a/libio/wfileops.c b/libio/wfileops.c
index 73d7709..99f9c8f 100644
--- a/libio/wfileops.c
+++ b/libio/wfileops.c
@@ -118,7 +118,7 @@ _IO_wdo_write (_IO_FILE *fp, const wchar_t *data, _IO_size_t to_do)
 	     fp->_wide_data->_IO_buf_base);
   fp->_wide_data->_IO_write_base = fp->_wide_data->_IO_write_ptr
     = fp->_wide_data->_IO_buf_base;
-  fp->_wide_data->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+  fp->_wide_data->_IO_write_end = ((fp->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
 				   ? fp->_wide_data->_IO_buf_base
 				   : fp->_wide_data->_IO_buf_end);
 
@@ -216,7 +216,7 @@ _IO_wfile_underflow (_IO_FILE *fp)
 
   /* Flush all line buffered files before reading. */
   /* FIXME This can/should be moved to genops ?? */
-  if (fp->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
+  if (fp->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
     {
 #if 0
       _IO_flush_all_linebuffered ();
@@ -477,7 +477,7 @@ _IO_wfile_overflow (_IO_FILE *f, wint_t wch)
       f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
 
       f->_flags |= _IO_CURRENTLY_PUTTING;
-      if (f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
+      if (f->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
 	f->_wide_data->_IO_write_end = f->_wide_data->_IO_write_ptr;
     }
   if (wch == WEOF)

[-- Attachment #3: test_flag.c --]
[-- Type: text/x-csrc, Size: 678 bytes --]

/*
 * =====================================================================================
 *
 *       Filename:  test.c
 *
 *    Description:  
 *
 *        Version:  1.0
 *        Created:  07/07/2015 11:14:49 PM
 *       Revision:  none
 *       Compiler:  gcc
 *
 *         Author:  YOUR NAME (), 
 *        Company:  
 *
 * =====================================================================================
 */
#include <stdlib.h>
#include <stdio.h>

#define _IO_UNBUFFERED 2
#define _IO_LINE_BUF 0x200

int main(void)
{
	if ((_IO_UNBUFFERED + _IO_LINE_BUF) == (_IO_UNBUFFERED | _IO_LINE_BUF)) {
		printf("Equal\n");
	} else {
		printf("Not equal\n");
	}

	return 0;
}

[-- Attachment #4: test_buf.c --]
[-- Type: text/x-csrc, Size: 729 bytes --]

/*
 * =====================================================================================
 *
 *       Filename:  test_buf.c
 *
 *    Description:  
 *
 *        Version:  1.0
 *        Created:  07/08/2015 10:43:01 AM
 *       Revision:  none
 *       Compiler:  gcc
 *
 *         Author:  YOUR NAME (), 
 *        Company:  
 *
 * =====================================================================================
 */
#include <stdlib.h>
#include <stdio.h>


int main(void)
{
	char buffer[256];

	printf("Before no-buffer.");
	printf("Output now.\n");

	setbuf(stdout, NULL);
	printf("No buffer now.");

	setvbuf(stdout, buffer, _IOLBF, sizeof(buffer));
	printf("Restore linebuf.");
	printf("Output now.\n");

	return 0;
}

Re: [PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[Feng Gao]

Hi Siddhesh,

Now I get the reason why the "make check" hang.
The "make check" need start after make. (I am a little curious why it
will hang if not make )

But the result is same whatever apply my change or not, it stops the
same step following:

gcc inl-tester.c -c -std=gnu99 -fgnu89-inline -fno-stack-protector -O2
-Wall -Werror -Wno-error=undef -Wundef -Wwrite-strings
-fmerge-all-constants -frounding-math -g -Wstrict-prototypes
-Wa,-mtune=i686 -fno-builtin    -U_FORTIFY_SOURCE   -I../include
-I/home/fgao/works/glibc-build/string  -I/home/fgao/works/glibc-build
-I../sysdeps/unix/sysv/linux/i386/i686  -I../sysdeps/i386/i686/nptl
-I../sysdeps/unix/sysv/linux/i386  -I../sysdeps/unix/sysv/linux/x86
-I../sysdeps/i386/nptl  -I../sysdeps/unix/sysv/linux/include
-I../sysdeps/unix/sysv/linux  -I../sysdeps/nptl  -I../sysdeps/pthread
-I../sysdeps/gnu  -I../sysdeps/unix/inet  -I../sysdeps/unix/sysv
-I../sysdeps/unix/i386  -I../sysdeps/unix  -I../sysdeps/posix
-I../sysdeps/i386/i686/fpu/multiarch  -I../sysdeps/i386/i686/fpu
-I../sysdeps/i386/i686/multiarch  -I../sysdeps/i386/i686
-I../sysdeps/i386/i486  -I../sysdeps/i386/fpu
-I../sysdeps/x86/fpu/include -I../sysdeps/x86/fpu  -I../sysdeps/i386
-I../sysdeps/x86  -I../sysdeps/wordsize-32
-I../sysdeps/ieee754/ldbl-96  -I../sysdeps/ieee754/dbl-64
-I../sysdeps/ieee754/flt-32  -I../sysdeps/ieee754
-I../sysdeps/generic  -I.. -I../libio -I.   -D_LIBC_REENTRANT -include
/home/fgao/works/glibc-build/libc-modules.h -DMODULE_NAME=nonlib
-include ../include/libc-symbols.h       -o
/home/fgao/works/glibc-build/string/inl-tester.o -MD -MP -MF
/home/fgao/works/glibc-build/string/inl-tester.o.dt -MT
/home/fgao/works/glibc-build/string/inl-tester.o
In file included from inl-tester.c:6:0:
tester.c: In function ‘test_memcmp’:
tester.c:1097:7: error: value computed is not used [-Werror=unused-value]
tester.c:1098:7: error: value computed is not used [-Werror=unused-value]
cc1: all warnings being treated as errors
make[2]: *** [/home/fgao/works/glibc-build/string/inl-tester.o] Error 1
make[2]: Leaving directory `/home/fgao/works/glibc/string'
make[1]: *** [string/tests] Error 2
make[1]: Leaving directory `/home/fgao/works/glibc'
make: *** [check] Error 2




On Wed, Jul 8, 2015 at 2:15 PM, Feng Gao <gfree.wind@gmail.com> wrote:
> Hi Siddhesh,
>
> Firstly, sorry about that I forget fixing the last space issue. Maybe
> I was very tried when it was very late in the night.
>
> Now the attachment "file_flags.diff" is the latest change which fix
> the issue you mentioned.
> The "test_flag.c" is the test codes to check the flag value.
> The "test_buf.c" is the test codes to test if the setbuf of libc works well.
>
> The following are the tests I did:
> 1. Run the test_flag.c on 32-bits and 64-bits platform, the output is "Equal".
>     It means (_IO_UNBUFFERED | _IO_LINE_BUF) has the same value of
> (_IO_UNBUFFERED + _IO_LINE_BUF);
> 2. After update the glibc, gdb the test_buf.c to check if the setbuf
> works well like before.
>     Because i am using the glibc-2.19 on my linux, so I apply my patch
> to glibc-2.19, build and replace the original one. ( I am afraid my
> computer become brick:))
>     Then gdb the binary of test_buf.c to check if the setbuf works
> well like before.
> Breakpoint 1, main () at test_buf.c:23
> 23      {
> (gdb) n
> 26              printf("Before no-buffer.");
> (gdb)
> 27              printf("Output now.\n");
> (gdb)
> Before no-buffer.Output now.
> 29              setbuf(stdout, NULL);
> (gdb)
> 30              printf("No buffer now.");
> (gdb)
> No buffer now.32                setvbuf(stdout, buffer, _IOLBF, sizeof(buffer));
> (gdb)
> 33              printf("Restore linebuf.");
> (gdb)
> 34              printf("Output now.\n");
> (gdb)
> Restore linebuf.Output now.
> 36              return 0;
>
> The output is the right behavior we expect.
>
> 3. I doesn't execute the "make check". Because it will hang without my
> patch. So I could not do "make check" with my patch.
> I don't know if there already is one bug in the current glibc codes.
>
> It stop the following step for about 2 hours, so I have to cancel it.
>
>         scripts/evaluate-test.sh c++-types-check $? false false >
> /home/fgao/works/my_git_codes/glibc-build/c++-types-check.test-result
> AWK='gawk' scripts/check-local-headers.sh \
>           "/usr/include" "/home/fgao/works/my_git_codes/glibc-build/"
>> /home/fgao/works/my_git_codes/glibc-build/check-local-headers.out; \
>         scripts/evaluate-test.sh check-local-headers $? false false >
> /home/fgao/works/my_git_codes/glibc-build/check-local-headers.test-result
>
> That's why I did not execute the "make check" with my change.
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
> On Wed, Jul 8, 2015 at 12:28 AM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
>> On Wed, Jul 08, 2015 at 12:02:38AM +0800, Feng Gao wrote:
>>> About the tests, I did the following cases:
>>> 1. Write test codes to check if the (_IO_LINE_BUF|_IO_UNBUFFERED)
>>> equals  (_IO_LINE_BUF+_IO_UNBUFFERED);
>>> 2. Update the glibc to check if it works like before.
>>
>> Thanks, it looks like you missed fixing spacing in the last instance
>> (quoted below).  Also, run 'make check' before and after the patch to
>> make sure that there are no regressions due to this change.  Updating
>> glibc is a brave thing to do - if it breaks, your box is a brick that
>> only a rescue disk can get back :)
>>
>> Please post an updated patch and also let me know the results of your
>> test.
>>
>> Thanks,
>> Siddhesh
>>
>>> @@ -477,7 +477,7 @@ _IO_wfile_overflow (_IO_FILE *f, wint_t wch)
>>>        f->_IO_read_base = f->_IO_read_ptr = f->_IO_read_end;
>>>
>>>        f->_flags |= _IO_CURRENTLY_PUTTING;
>>> -      if (f->_flags & (_IO_LINE_BUF+_IO_UNBUFFERED))
>>> +      if (f->_flags & (_IO_LINE_BUF|_IO_UNBUFFERED))
>>>       f->_wide_data->_IO_write_end = f->_wide_data->_IO_write_ptr;
>>>      }
>>>    if (wch == WEOF)
>>

Re: [PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[Siddhesh Poyarekar]

On Wed, Jul 08, 2015 at 04:11:19PM +0800, Feng Gao wrote:
> Now I get the reason why the "make check" hang.
> The "make check" need start after make. (I am a little curious why it
> will hang if not make )
> 
> But the result is same whatever apply my change or not, it stops the
> same step following:

That is strange - I don't see that.  In any case, the generated
libc.so with or without the patch is identical (tested by comparing
outputs of objdump -d libc.so), so I'm going to push this change.

Thanks,
Siddhesh

Re: [PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[Feng Gao]

Hi Siddhesh,

Does it mean you have accepted my change ?

The results of "make check" are same whatever apply my change or not
in the last email.

The following are the platform details:
1. gcc version: (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3
2. Linux kernel vesion: 3.2.0-80
3. cpu info:Intel(R) Core(TM) i7-2600

When I run "make check" on another platform (32bits), I get different results.
1. Without my change:
FAIL: elf/check-abi-libc
FAIL: elf/tst-protected1a
FAIL: elf/tst-protected1b
XPASS: elf/tst-tlsalign
XPASS: elf/tst-tlsalign-static
FAIL: localedata/sort-test
FAIL: nptl/tst-cond21
FAIL: nptl/tst-setuid3
FAIL: stdlib/tst-makecontext
Summary of test results:
      7 FAIL
   2278 PASS
     87 XFAIL
      5 XPASS

2. With my change:
FAIL: elf/check-abi-libc
FAIL: elf/tst-protected1a
FAIL: elf/tst-protected1b
XPASS: elf/tst-tlsalign
XPASS: elf/tst-tlsalign-static
FAIL: malloc/tst-malloc-backtrace
FAIL: nptl/tst-setuid3
FAIL: stdlib/tst-makecontext
Summary of test results:
      6 FAIL
   2279 PASS
     87 XFAIL
      5 XPASS

The details of this platform are following:
1. gcc version:(Ubuntu 4.8.2-19ubuntu1) 4.8.2
2. Linux kernel version: Linux ubuntu 4.0.6+
3. cpu info: Intel(R) Core(TM) i5-4200U CPU

On Wed, Jul 8, 2015 at 4:22 PM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
> On Wed, Jul 08, 2015 at 04:11:19PM +0800, Feng Gao wrote:
>> Now I get the reason why the "make check" hang.
>> The "make check" need start after make. (I am a little curious why it
>> will hang if not make )
>>
>> But the result is same whatever apply my change or not, it stops the
>> same step following:
>
> That is strange - I don't see that.  In any case, the generated
> libc.so with or without the patch is identical (tested by comparing
> outputs of objdump -d libc.so), so I'm going to push this change.
>
> Thanks,
> Siddhesh

Re: [PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[Carlos O'Donell]

On 07/08/2015 02:15 AM, Feng Gao wrote:
> It stop the following step for about 2 hours, so I have to cancel it.
> 
>         scripts/evaluate-test.sh c++-types-check $? false false >
> /home/fgao/works/my_git_codes/glibc-build/c++-types-check.test-result
> AWK='gawk' scripts/check-local-headers.sh \
>           "/usr/include" "/home/fgao/works/my_git_codes/glibc-build/"
>> /home/fgao/works/my_git_codes/glibc-build/check-local-headers.out; \
>         scripts/evaluate-test.sh check-local-headers $? false false >
> /home/fgao/works/my_git_codes/glibc-build/check-local-headers.test-result

This is a super annoying failure mode that happens when `make` fails or
is not run. The fix is as follows, and I'll check it in shortly.

2015-07-08  Carlos O'Donell  <carlos@redhat.com>

	* Makefile ($(objpfx)check-local-headers.out):
	Redirect input from /dev/null.

diff --git a/Makefile b/Makefile
index 658ccfa..c88b2e5 100644
--- a/Makefile
+++ b/Makefile
@@ -262,7 +262,7 @@ endif
 
 $(objpfx)check-local-headers.out: scripts/check-local-headers.sh
 	AWK='$(AWK)' scripts/check-local-headers.sh \
-	  "$(includedir)" "$(objpfx)" > $@; \
+	  "$(includedir)" "$(objpfx)" < /dev/null > $@; \
 	$(evaluate-test)
 
 ifneq ($(PERL),no)
--

c.

Re: [PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[Siddhesh Poyarekar]

On 8 July 2015 at 14:14, Feng Gao <gfree.wind@gmail.com> wrote:
> Does it mean you have accepted my change ?

Yes.

Siddhesh

-- 
http://siddhesh.in

Re: [PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[Siddhesh Poyarekar]

On 8 July 2015 at 14:25, Carlos O'Donell <carlos@redhat.com> wrote:
> This is a super annoying failure mode that happens when `make` fails or
> is not run. The fix is as follows, and I'll check it in shortly.
>
> 2015-07-08  Carlos O'Donell  <carlos@redhat.com>
>
>         * Makefile ($(objpfx)check-local-headers.out):
>         Redirect input from /dev/null.
>
> diff --git a/Makefile b/Makefile
> index 658ccfa..c88b2e5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -262,7 +262,7 @@ endif
>
>  $(objpfx)check-local-headers.out: scripts/check-local-headers.sh
>         AWK='$(AWK)' scripts/check-local-headers.sh \
> -         "$(includedir)" "$(objpfx)" > $@; \
> +         "$(includedir)" "$(objpfx)" < /dev/null > $@; \
>         $(evaluate-test)

That looks like a hack.  Does it hang because $(objpfx) and
$(includedir) are not set?  A better fix ought to be to ensure that
either 'make check' invokes make (and hence sets things up for the
check target) or it fails early and cleanly, i.e. irrespective of
whether the check-local-headers test is run or not.

Siddhesh
-- 
http://siddhesh.in

Re: [PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[Andreas Schwab]

"Carlos O'Donell" <carlos@redhat.com> writes:

> diff --git a/Makefile b/Makefile
> index 658ccfa..c88b2e5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -262,7 +262,7 @@ endif
>  
>  $(objpfx)check-local-headers.out: scripts/check-local-headers.sh
>  	AWK='$(AWK)' scripts/check-local-headers.sh \
> -	  "$(includedir)" "$(objpfx)" > $@; \
> +	  "$(includedir)" "$(objpfx)" < /dev/null > $@; \
>  	$(evaluate-test)

This isn't the only problem with check-local-headers.  It should really
make sure all *.dt files have been converted to *.d files first.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[Andreas Schwab]

Siddhesh Poyarekar <siddhesh.poyarekar@gmail.com> writes:

> That looks like a hack.  Does it hang because $(objpfx) and
> $(includedir) are not set?

No, it hangs because */*.{o,os,oS}.d expands to nothing.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[Carlos O'Donell]

On 07/08/2015 05:44 AM, Siddhesh Poyarekar wrote:
> On 8 July 2015 at 14:25, Carlos O'Donell <carlos@redhat.com> wrote:
>> This is a super annoying failure mode that happens when `make` fails or
>> is not run. The fix is as follows, and I'll check it in shortly.
>>
>> 2015-07-08  Carlos O'Donell  <carlos@redhat.com>
>>
>>         * Makefile ($(objpfx)check-local-headers.out):
>>         Redirect input from /dev/null.
>>
>> diff --git a/Makefile b/Makefile
>> index 658ccfa..c88b2e5 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -262,7 +262,7 @@ endif
>>
>>  $(objpfx)check-local-headers.out: scripts/check-local-headers.sh
>>         AWK='$(AWK)' scripts/check-local-headers.sh \
>> -         "$(includedir)" "$(objpfx)" > $@; \
>> +         "$(includedir)" "$(objpfx)" < /dev/null > $@; \
>>         $(evaluate-test)
> 
> That looks like a hack.  Does it hang because $(objpfx) and
> $(includedir) are not set?  A better fix ought to be to ensure that
> either 'make check' invokes make (and hence sets things up for the
> check target) or it fails early and cleanly, i.e. irrespective of
> whether the check-local-headers test is run or not.

It's not a hack. The script reads from stdin for the list of *.d
files that were created by the build, but if there are no *.d files
the shell expansion is empty and that forces the script to read from
the parent's inherited stdin. Since the parent never writes anything
to stdin the awk script blocks on the read forever. As Andreas notes
it should dep on the *.d files existing, and that is true. However,
even if that were fixed we should still read from /dev/null and
fail the test even if all the *.d files were deleted between the
time they were created and the script ran (robust).

Cheers,
Carlos.

Re: [PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[Siddhesh Poyarekar]

On 8 July 2015 at 18:16, Carlos O'Donell <carlos@redhat.com> wrote:
> It's not a hack. The script reads from stdin for the list of *.d
> files that were created by the build, but if there are no *.d files
> the shell expansion is empty and that forces the script to read from
> the parent's inherited stdin. Since the parent never writes anything
> to stdin the awk script blocks on the read forever. As Andreas notes
> it should dep on the *.d files existing, and that is true. However,
> even if that were fixed we should still read from /dev/null and
> fail the test even if all the *.d files were deleted between the
> time they were created and the script ran (robust).

Right, I misread what that target was doing.  In any case, the right
fix here would be to make the check-local-headers target depend on *.d
(one of them I guess?) in a way that they're always generated before
the script is run.

Siddhesh
-- 
http://siddhesh.in

Re: [PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[Carlos O'Donell]

On 07/08/2015 09:09 AM, Siddhesh Poyarekar wrote:
> On 8 July 2015 at 18:16, Carlos O'Donell <carlos@redhat.com> wrote:
>> It's not a hack. The script reads from stdin for the list of *.d
>> files that were created by the build, but if there are no *.d files
>> the shell expansion is empty and that forces the script to read from
>> the parent's inherited stdin. Since the parent never writes anything
>> to stdin the awk script blocks on the read forever. As Andreas notes
>> it should dep on the *.d files existing, and that is true. However,
>> even if that were fixed we should still read from /dev/null and
>> fail the test even if all the *.d files were deleted between the
>> time they were created and the script ran (robust).
> 
> Right, I misread what that target was doing.  In any case, the right
> fix here would be to make the check-local-headers target depend on *.d
> (one of them I guess?) in a way that they're always generated before
> the script is run.

I agree that `make check` should effectively run `make` if the
dependencies are not yet met, but we aren't there yet, and I see
a whole host of other failures to fix. It's also not obvious to me how
I would depend on *all* *.d files in that makefile target, it doesn't seem
trivial to compute them. The fix to read from /dev/null seems like
the lowest cost immediate solution that yields a `make check` failure
rather than a hang (which is terrible for automated test systems also).

If nobody objects I'll check it in now to p

Cheers,
Carlos.

Re: [PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[Andreas Schwab]

Siddhesh Poyarekar <siddhesh.poyarekar@gmail.com> writes:

> Right, I misread what that target was doing.  In any case, the right
> fix here would be to make the check-local-headers target depend on *.d
> (one of them I guess?) in a way that they're always generated before
> the script is run.

It would need to depend on a target that recurses through all subdirs.
The %.d:%.dt rule is triggered by include $(+depfiles).

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[Andreas Schwab]

"Carlos O'Donell" <carlos@redhat.com> writes:

> The fix to read from /dev/null seems like the lowest cost immediate
> solution that yields a `make check` failure rather than a hang (which
> is terrible for automated test systems also).

Alternatively, add /dev/null to the awk command line.  That would also
make the comment true:

# OK if *.os is missing.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Use "|" instead of "+" when combine the _IO_LINE_BUF and _IO_UNBUFFERED flags

[Joseph Myers]

On Wed, 8 Jul 2015, Andreas Schwab wrote:

> Siddhesh Poyarekar <siddhesh.poyarekar@gmail.com> writes:
> 
> > Right, I misread what that target was doing.  In any case, the right
> > fix here would be to make the check-local-headers target depend on *.d
> > (one of them I guess?) in a way that they're always generated before
> > the script is run.
> 
> It would need to depend on a target that recurses through all subdirs.
> The %.d:%.dt rule is triggered by include $(+depfiles).

I'd say that check-local-headers, instead of being a top-level test at 
all, should be a test present automatically in each subdirectory that 
checks only files in that subdirectory (and so doesn't need to depend on 
anything that recurses, only on something within that directory).  See 
what I said in 
<https://sourceware.org/ml/libc-alpha/2014-01/msg00197.html> about 
eliminating top-level tests.

-- 
Joseph S. Myers
joseph@codesourcery.com