bugmake - Bugs: bug #34832, enable tls variable in windows...

 
 

bug #34832: enable tls variable in windows code for newish gcc

Submitter:  Ozkan Sezer <sezero>
Submitted:  Tue 15 Nov 2011 04:06:15 PM UTC
   
 
Severity:  3 - Normal Item Group:  None
Status:  Fixed Privacy:  Public
Assigned to:  eliz Open/Closed:  Closed
Component Version:  4.0 Operating System:  MS Windows
Fixed Release:  4.0 Triage Status:  Small Effort
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 03 Feb 2012 12:19:52 PM UTC, comment #23: 

I fixed this by removing the TLS declaration for MSVC, leaving behind a comment about that.

I also ifdef'ed away the special code for Winsock errors, let's see if that breaks something.

In a separate commit, I also ifdef'ed away unused functions in sub_proc.c, which made it look as if several threads are being used by GNU Make on MS-Windows.

Eli Zaretskii <eliz>
Group Member
Wed 16 Nov 2011 08:31:52 AM UTC, comment #22: 

Propagating the tls functionality to gcc-builds won't hurt, but as I already said, please just remove it altogether if it isn't helping with anything (which it certainly looks that way) and that will stop any future confusions.

Ozkan Sezer <sezero>
Wed 16 Nov 2011 08:14:13 AM UTC, comment #21: 

Ozkan,

I don't understand the emotions.  This is supposed to be a technical discussion.  If I said something that sounded offensive, I apologize.

More to the point: I don't know why it is there.  1996 is way before I started to be interested in the Windows port of Make, certainly well before I began contributing.  ChangeLog says that my first contribution to the Windows port was in 2005.

"cvs annotate" shows that this TLS thing was in w32err.c since its first version was contributed by Rob Tulloh when the Windows port was first added to Make.  The commented-out calls to map_win32_error_to_string were already commented-out in that original commit, again according to "cvs annotate".

Moreover, as someone already mentioned here, process_pipe_io, the only function that launches additional threads, is never called by Make.  (I will ifdef it away when I have time.)

The only situation I'm aware of when more than a single thread is running in Make is when you hit Ctrl-C while Make runs, which on Windows creates a separate thread to run the signal handler.  But the first thing that handler does is stop the main thread in its tracks, so that the two threads don't step on each other's toes.

So, as far as Make is concerned, the situation with several threads trying to write to the same static buffer was never tested, neither with MSVC nor with GCC.

Given all this, if someone still thinks we should enable TLS for GCC, I'm willing to accept the patch, since it obviously cannot do any harm.

Eli Zaretskii <eliz>
Group Member
Tue 15 Nov 2011 09:01:35 PM UTC, comment #20: 


> As for TLS availability for MSVC being a proof that it works,
> I have my doubts:


Well, why the hell is it there, then?  I assume you are the one
who is supposed to know that, and not me.  If it is unnecessary
or obsolete or whatever, removing it will eliminate any further
future confusions.

> with most of calls to the function commented away,


If we are talking about sub_proc.c, there is only one active call
but it doesn't do anything with the returned string.

Ozkan Sezer <sezero>
Tue 15 Nov 2011 08:35:12 PM UTC, comment #19: 

You can provide a stack buffer from the caller like
  char * tmp[1000];
  fprintf(..., map_windows32_error_to_string (tmp, er));

And the functions that call map_windows32_error_to_string from threads are not used.

Anonymous
Tue 15 Nov 2011 08:28:35 PM UTC, comment #18: 

All the uses pointed out by Paul are from a single thread.  The only problem, if there is one, is with the calls from sub_proc.c.

As for TLS availability for MSVC being a proof that it works, I have my doubts: with most of calls to the function commented away, an argument could be made that it does not work, and was commented out because of that.

It doesn't make much sense to have a possibility of simultaneous calls, and forcibly comment out those calls at the same time.

Eli Zaretskii <eliz>
Group Member
Tue 15 Nov 2011 08:21:28 PM UTC, comment #17: 

It is used:

make-cvs$ grep map_windows32_error_to_string *.c
commands.c:                ierr, map_windows32_error_to_string (ierr));
job.c:              error_string = map_windows32_error_to_string (er);
job.c:                             e, map_windows32_error_to_string(e));
job.c:                          exit_code, map_windows32_error_to_string(exit_code));
job.c:                 err, map_windows32_error_to_string(err));
job.c:                   err, map_windows32_error_to_string(err));
job.c:                  err, exit_code, map_windows32_error_to_string(err));
main.c:                 cp, err, map_windows32_error_to_string(err));
main.c:                 err, map_windows32_error_to_string(err));


And, note, not in ways in which it's easy to capture a returned dynamic buffer and free it afterwards.  It would involve adding multiple lines of code for each call.

Paul D. Smith <psmith>
Group administrator
Tue 15 Nov 2011 08:14:32 PM UTC, comment #16: 

You can't easily simulate errors in a function that is not used from anywhere.

Anonymous
Tue 15 Nov 2011 08:09:23 PM UTC, comment #15: 


> We could get rid of that static buffer, or we could use the
> __thread declaration, but my point is: if we are enabling to
> produce error message strings from several threads, we might
> as well actually do that and see that it works, e.g., by
> uncommenting the commented-out calls and simulating error
> conditions in several threads simultaneously.
>
> IOW, if we want to have this feature, we might as well make
> sure it works.


Well, tls for MSVC is there since 1996. And I assume that it is
working since you actually have it there.  If you want to
propagate the functionality to gcc as I originally intended, you
have the patch already, what you want to test seems to be whether
gcc+binutils works for you..

Ozkan Sezer <sezero>
Tue 15 Nov 2011 08:02:43 PM UTC, comment #14: 


> if the benefits are worth the complexity.


IMO, definitely not.

Side notes: the winsock errors check can definitely be removed
from there along with the tls usage for msvc special case. And
the function needs returning const.

Ozkan Sezer <sezero>
Tue 15 Nov 2011 08:01:54 PM UTC, comment #13: 

We could get rid of that static buffer, or we could use the __thread declaration, but my point is: if we are enabling to produce error message strings from several threads, we might as well actually do that and see that it works, e.g., by uncommenting the commented-out calls and simulating error conditions in several threads simultaneously.

IOW, if we want to have this feature, we might as well make sure it works.

Eli Zaretskii <eliz>
Group Member
Tue 15 Nov 2011 07:59:15 PM UTC, comment #12: 

The reason the buffer is static is that if you allocate a dynamic buffer, someone has to free it.  In POSIX we use the strerror() function which returns a const pointer to a static string that never needs to be freed so there's no facility in the code currently for freeing the error string buffer.

It could be done but it seems like it would add a lot of complexity.  Not sure if the benefits are worth the complexity.

Paul D. Smith <psmith>
Group administrator
Tue 15 Nov 2011 07:46:30 PM UTC, comment #11: 


> Why don't you rewrite the code do get rid of that static buffer
> in the first place.


That's certainly a good idea (had we known who you are, it would
been even better, I guess.)  Another cleanup would be removing
the pointless winsock error code checks in there.

Do you have a patch?

Ozkan Sezer <sezero>
Tue 15 Nov 2011 07:36:50 PM UTC, comment #10: 

Why don't you rewrite the code do get rid of that static buffer in the first place.

Anonymous
Tue 15 Nov 2011 06:58:47 PM UTC, comment #9: 

Then __declspec(thread) isn't actually needed, not for msvc, not for gcc either.   As for thread-safety, I first though that that I caught glimpses of CreateThread(), but hmm, those turned out to be CreateProcess().  There are calls to _beginthreadex() in sub_proc.c, but they are for pipe emulation as I understand (I may very well be wrong), so multithread safety may indeed be not necessary for map_windows32_error_to_string().

Ozkan Sezer <sezero>
Tue 15 Nov 2011 06:49:04 PM UTC, comment #8: 

Paul,

The Windows build of Make does use more than a single thread.

Ozkan,

Yes, I know that functions in the top-level directory call map_windows32_error_to_string, but those functions all run in a single thread.  The only functions that don't are in sub_proc.c.

Eli Zaretskii <eliz>
Group Member
Tue 15 Nov 2011 06:41:01 PM UTC, comment #7: 

That function is used a lot in job.c, main.c, commands.c.

But I agree with Eli: what's the need for having this variable be thread-safe in the first place?  GNU make is not multithreaded (there would be a LOT of work needed to make that possible).  Is there some attribute of the W32 API, or the way this variable is used, that requires it to be thread-local?

Maybe the right thing to do is remove the existing thread safety stuff, not enhancing it for new compilers...?  But I don't know enough about Windows to say.

Paul D. Smith <psmith>
Group administrator
Tue 15 Nov 2011 06:36:09 PM UTC, comment #6: 

Are you looking at code from 1996?  Have ever you tried grepping under top level *.c files for map_windows32_error_to_string ?

Ozkan Sezer <sezero>
Tue 15 Nov 2011 06:30:02 PM UTC, comment #5: 

Which multiple threads in Make might try to access the same static buffer concurrently?  And under what circumstances?  All but one call to map_windows32_error_to_string in sub_proc.c are commented out.

Eli Zaretskii <eliz>
Group Member
Tue 15 Nov 2011 06:06:24 PM UTC, comment #4: 

Because at the time that code was writtten, a __declspec(thread) equivalent wasn't available for gcc: that's the original author's  skills in making a useful comment you are seeing. TLS is obviously necessary for that static array to make multiple threads not try to access the same thing concurrently.

Ozkan Sezer <sezero>
Tue 15 Nov 2011 05:57:38 PM UTC, comment #3: 

Thanks.  I know what TLS means.  I meant to ask why it is useful for map_windows32_error_to_string to use thread local storage?  What will that gain for Make?  The comment there said it was only needed for MSVC.

Eli Zaretskii <eliz>
Group Member
Tue 15 Nov 2011 05:20:37 PM UTC, comment #2: 

The patch enables thread local storage in map_windows32_error_to_string() not only for MSVC, but also for gcc-built make binaries. It also constifies the returned value from the function, because the returned string is always used as read-only arguments in printf format strings.

As for the whitespace changes, I made the 'else' case of the function consistent with its 'if' counterpart; can be simply ignored, of course.

Ozkan Sezer <sezero>
Tue 15 Nov 2011 05:12:44 PM UTC, comment #1: 

Thanks.

However, you didn't explain what does this patch try to accomplish, and there's no ChangeLog entry in sight to help me understand that.  Could you please describe the rationale?

Also, please don't make gratuitous changes of whitespace, they make it harder to see the actual changes.

Eli Zaretskii <eliz>
Group Member
Tue 15 Nov 2011 04:06:15 PM UTC, original submission:  

Hi:

The attached patch enables tls variable usage in
map_windows32_error_to_string() for newish gcc.

While I was there, I also constified the function's
return which is obviously the intended case.

Please review and consider for applying.  Regards.

Ozkan Sezer <sezero>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #24381:  make-TLS_VAR.patch added by sezero (3KiB - application/octet-stream - enable TLS variable for newish gcc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by psmith (Posted a comment)
  • -email is unavailable- added by eliz (Posted a comment)
  • -email is unavailable- added by sezero (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2012-02-03 eliz StatusNone Fixed
    2012-02-03 eliz Open/ClosedOpen Closed
        Fixed ReleaseNone 4.0
    2012-01-29 psmith Assigned toNone eliz
        Triage StatusNone Small Effort
    2011-11-15 sezero Attached File- Added make-TLS_VAR.patch, #24381

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code