Bug 160195 - check-for-exit-time-destructors should be usable outside Xcode
Summary: check-for-exit-time-destructors should be usable outside Xcode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-26 05:52 PDT by David Kilzer (:ddkilzer)
Modified: 2016-07-28 12:56 PDT (History)
4 users (show)

See Also:


Attachments
WIP Patch (4.61 KB, patch)
2016-07-26 05:52 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v1 (5.59 KB, patch)
2016-07-26 09:25 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2016-07-26 05:52:35 PDT
Created attachment 284583 [details]
WIP Patch

It would be nice if check-for-exit-time-destructors was generally usable outside an Xcode build phase script.
Comment 1 David Kilzer (:ddkilzer) 2016-07-26 09:25:17 PDT
Created attachment 284597 [details]
Patch v1
Comment 2 David Kilzer (:ddkilzer) 2016-07-26 09:25:52 PDT
(In reply to comment #1)
> Created attachment 284597 [details]
> Patch v1

Same as "WIP Patch", but with ChangeLog.
Comment 3 David Kilzer (:ddkilzer) 2016-07-26 09:35:06 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > Created attachment 284597 [details]
> > Patch v1
> 
> Same as "WIP Patch", but with ChangeLog.

https://bugs.webkit.org/attachment.cgi?oldid=284583&action=interdiff&newid=284597&headers=1
Comment 4 Darin Adler 2016-07-26 14:58:13 PDT
Comment on attachment 284597 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=284597&action=review

> Tools/ChangeLog:13
> +        * Scripts/check-for-exit-time-destructors: Update to parse
> +        -h|--help switch, or to take one argument to a binary to check
> +        for exit time destructors on the command-line.  The clang
> +        compiler will find these at compile-time with the
> +        -Wexit-time-destructors switch, but this script will check for
> +        them after-the-fact.

Given that -Wexit-time-destructors now exists, do we even need this script at all any more?

> Tools/Scripts/check-for-exit-time-destructors:65
> +    # Running from Xcode build phase script with no arguments.

Maybe this behavior should be triggered by an explicit command line argument instead of by "no arguments".

Also, when you try this and none of the environment variables are set, what ends up happening?
Comment 5 WebKit Commit Bot 2016-07-26 15:05:13 PDT
Comment on attachment 284597 [details]
Patch v1

Clearing flags on attachment: 284597

Committed r203742: <http://trac.webkit.org/changeset/203742>
Comment 6 WebKit Commit Bot 2016-07-26 15:05:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 David Kilzer (:ddkilzer) 2016-07-27 17:17:15 PDT
Comment on attachment 284597 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=284597&action=review

>> Tools/ChangeLog:13
>> +        them after-the-fact.
> 
> Given that -Wexit-time-destructors now exists, do we even need this script at all any more?

The script is not referenced in any Xcode project in WebKit anymore, but the script was very useful in checking for exit-time destructors in a compiled binary (see <rdar://problem/27528158>).

Not sure if the WebKit project is the right place to host such a script, though.  (The check-for-global-initializers script also has a clang warning now.)

Thoughts?

>> Tools/Scripts/check-for-exit-time-destructors:65
>> +    # Running from Xcode build phase script with no arguments.
> 
> Maybe this behavior should be triggered by an explicit command line argument instead of by "no arguments".
> 
> Also, when you try this and none of the environment variables are set, what ends up happening?

The script prints a bunch of error messages, then exits without doing anything useful.

A command-line switch would make it not spew error messages, which would be a better user experience.
Comment 8 Darin Adler 2016-07-28 08:46:34 PDT
I personally think that this script, which is one that I wrote to track progress of the project to eliminate all exit-time destructors from WebKit specifically to help with an iOS-specific issue (see bug 22061), has outlived its usefulness and should be deleted.
Comment 9 Darin Adler 2016-07-28 08:56:41 PDT
Oh, and I now see how this script was useful internally at Apple, Dave. I agree, it should be moved somewhere else since it’s not important for building WebKit any more.
Comment 10 David Kilzer (:ddkilzer) 2016-07-28 12:56:28 PDT
(In reply to comment #8)
> I personally think that this script, which is one that I wrote to track
> progress of the project to eliminate all exit-time destructors from WebKit
> specifically to help with an iOS-specific issue (see bug 22061), has
> outlived its usefulness and should be deleted.

Filed Bug 160302 to track removing the scripts (and using -Wexit-time-destructors, -Wglobal-constructors consistently).  In hindsight, maybe I should have separated these two steps, though.

(In reply to comment #9)
> Oh, and I now see how this script was useful internally at Apple, Dave. I
> agree, it should be moved somewhere else since it’s not important for
> building WebKit any more.

I have an idea about next steps here.  More radars forthcoming.  :)