Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2255093004:
Make __DATE__ and __TIME__ to evaluate to an empty string. (Closed)
|
Created:
4 years, 4 months ago by Yoshisato Yanagisawa Modified:
3 years, 10 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake __DATE__ and __TIME__ to evaluate to an empty string.
If __DATE__ or __TIME__ used in code, current date or time will be
filled. Such a value may change compile by compile, and breaks
deterministic build.
BUG=314403
Committed: https://crrev.com/43088a9ef885a695061a49bcd0992cbce00a7568
Cr-Commit-Position: refs/heads/master@{#414018}
Patch Set 1 #Patch Set 2 : changed the default behavior. #
Total comments: 3
Patch Set 3 : force eliminate __DATE__ and __TIME__. #Patch Set 4 : cleaned up comments. #Patch Set 5 : rebase #
Total comments: 2
Messages
Total messages: 49 (16 generated)
yyanagisawa@chromium.org changed reviewers: + dpranke@google.com, maruel@chromium.org
+Nico since he probably has an opinion about it; he's OOO but comes back on Monday. Can you not make it a flag and just use this all the time? Context: this reduces object determinism with goma, which would help goma and is necessary for build determinism in Chromium.
On 2016/08/19 20:01:35, M-A Ruel wrote: > +Nico since he probably has an opinion about it; he's OOO but comes back on > Monday. > > Can you not make it a flag and just use this all the time? > > Context: this reduces object determinism with goma, which would help goma and is > necessary for build determinism in Chromium. That was going to be my question, also. This should just be on by default unless there's a reason we really need this?
Description was changed from ========== Add GN arg dont_embed_date_time to clear __DATE__ and __TIME__. If __DATE__ or __TIME__ used in code, current date or time will be filled. Such a value may change compile by compile, and breaks deterministic build. Let me provide the option to make such value always empty string. BUG=314403 ========== to ========== Add GN arg enable_date_time_builtin_macro. Let me make __DATE__ and __TIME__ eliminated by default. If __DATE__ or __TIME__ used in code, current date or time will be filled. Such a value may change compile by compile, and breaks deterministic build. Let me provide the option to make such value always empty string, and make it default. BUG=314403 ==========
On 2016/08/19 20:09:34, Dirk Pranke wrote: > On 2016/08/19 20:01:35, M-A Ruel wrote: > > +Nico since he probably has an opinion about it; he's OOO but comes back on > > Monday. > > > > Can you not make it a flag and just use this all the time? > > > > Context: this reduces object determinism with goma, which would help goma and > is > > necessary for build determinism in Chromium. > > That was going to be my question, also. This should just be on by default unless > there's a reason > we really need this? I changed the default behavior to kill __DATE__ and __TIME__ by default. I think this change is needed for deterministic build not only with goma but also without goma.
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
https://codereview.chromium.org/2255093004/diff/20001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2255093004/diff/20001/build/config/compiler/B... build/config/compiler/BUILD.gn:85: enable_date_time_builtin_macro = false Does this even need to be easily toggleable? Why not just unconditionally set the flags on line 217?
https://codereview.chromium.org/2255093004/diff/20001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2255093004/diff/20001/build/config/compiler/B... build/config/compiler/BUILD.gn:85: enable_date_time_builtin_macro = false On 2016/08/22 17:31:45, Dirk Pranke wrote: > Does this even need to be easily toggleable? Why not just unconditionally set > the flags on line 217? That's my opinion too, no need for a flag. Less flag is better.
https://codereview.chromium.org/2255093004/diff/20001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2255093004/diff/20001/build/config/compiler/B... build/config/compiler/BUILD.gn:85: enable_date_time_builtin_macro = false I do not have strong opinions on whether to make it flags or not make it flags. Currently, __DATE__ seems only show up to flatc and I do not think it meaningful to show actual date there. Let me force the metadata eliminated.
lgtm
Description was changed from ========== Add GN arg enable_date_time_builtin_macro. Let me make __DATE__ and __TIME__ eliminated by default. If __DATE__ or __TIME__ used in code, current date or time will be filled. Such a value may change compile by compile, and breaks deterministic build. Let me provide the option to make such value always empty string, and make it default. BUG=314403 ========== to ========== Make __DATE__ and __TIME__ to evaluate to an empty string. If __DATE__ or __TIME__ used in code, current date or time will be filled. Such a value may change compile by compile, and breaks deterministic build. BUG=314403 ==========
Rewrote the CL description. lgtm
The CQ bit was checked by maruel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2255093004/#ps60001 (title: "cleaned up comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Since this is inside !is_win, I do not think this affects windows build. Let me retry.
The CQ bit was checked by yyanagisawa@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by yyanagisawa@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, maruel@chromium.org Link to the patchset: https://codereview.chromium.org/2255093004/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch. On branch working_branch Your branch is up-to-date with 'origin/refs/pending/heads/master'. nothing to commit, working tree clean
Message was sent while issue was closed.
Description was changed from ========== Make __DATE__ and __TIME__ to evaluate to an empty string. If __DATE__ or __TIME__ used in code, current date or time will be filled. Such a value may change compile by compile, and breaks deterministic build. BUG=314403 ========== to ========== Make __DATE__ and __TIME__ to evaluate to an empty string. If __DATE__ or __TIME__ used in code, current date or time will be filled. Such a value may change compile by compile, and breaks deterministic build. BUG=314403 Committed: https://crrev.com/43088a9ef885a695061a49bcd0992cbce00a7568 Cr-Commit-Position: refs/heads/master@{#414018} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/43088a9ef885a695061a49bcd0992cbce00a7568 Cr-Commit-Position: refs/heads/master@{#414018}
Message was sent while issue was closed.
rohitrao@chromium.org changed reviewers: + rohitrao@chromium.org
Message was sent while issue was closed.
This appears to be crashing clang on all of the internal iOS bots. I can
reproduce with a local compile as well. I'm not sure why the upstream bots are
unaffected, might be something to do with the difference in xcode versions.
Is this something that can be reverted?
Assertion failed: (!StoredMD.getLatest() && "the macro history was modified
before initializing it from a pch"), function setLoadedMacroDirective, file
/b/build/slave/mac_upload_clang/build/src/third_party/llvm/tools/clang/lib/Lex/PPMacroExpansion.cpp,
line 74.
Stack dump:
0. Program arguments:
/Users/Shared/Chrome/ios/src/third_party/llvm-build/Release+Asserts/bin/clang
-cc1 -triple x86_64-apple-macosx10.9.0 -Wdeprecated-objc-isa-usage
-Werror=deprecated-objc-isa-usage -emit-obj -mrelax-all -disable-free
-main-file-name class-dump.m -mrelocation-model pic -pic-level 2 -mthread-model
posix -mdisable-fp-elim -relaxed-aliasing -masm-verbose -munwind-tables
-target-cpu core2 -target-linker-version 253.3 -dwarf-column-info
-debug-info-kind=standalone -dwarf-version=2 -debugger-tuning=lldb
-coverage-file
/Users/Shared/Chrome/ios/src/out/gn-Debug-iphonesimulator/clang_x64/obj/third_party/class-dump/class-dump/class-dump.o
-resource-dir
/Users/Shared/Chrome/ios/src/third_party/llvm-build/Release+Asserts/bin/../lib/clang/4.0.0
-dependency-file clang_x64/obj/third_party/class-dump/class-dump/class-dump.o.d
-MT clang_x64/obj/third_party/class-dump/class-dump/class-dump.o -isysroot
/Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk
-include-pch
clang_x64/obj/third_party/class-dump/class-dump/class-dump-Prefix.h-m.gch -D
V8_DEPRECATION_WARNINGS -D ENABLE_NOTIFICATIONS -D ENABLE_PEPPER_CDMS -D
ENABLE_PLUGINS=1 -D ENABLE_PDF=1 -D ENABLE_PRINTING=1 -D ENABLE_BASIC_PRINTING=1
-D ENABLE_PRINT_PREVIEW=1 -D ENABLE_SPELLCHECK=1 -D USE_BROWSER_SPELLCHECKER=1
-D NO_TCMALLOC -D USE_EXTERNAL_POPUP_MENU=1 -D ENABLE_WEBRTC=1 -D
ENABLE_EXTENSIONS=1 -D ENABLE_TASK_MANAGER=1 -D ENABLE_THEMES=1 -D
ENABLE_CAPTIVE_PORTAL_DETECTION=1 -D ENABLE_SESSION_SERVICE=1 -D
ENABLE_PLUGIN_INSTALLATION=1 -D ENABLE_SUPERVISED_USERS=1 -D
ENABLE_SERVICE_DISCOVERY=1 -D FULL_SAFE_BROWSING -D SAFE_BROWSING_CSD -D
SAFE_BROWSING_DB_LOCAL -D CHROMIUM_BUILD -D ENABLE_MEDIA_ROUTER=1 -D
FIELDTRIAL_TESTING_ENABLED -D CR_CLANG_REVISION=278861-1 -D
CR_XCODE_VERSION=0800 -D COMPONENT_BUILD -D
__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0 -D _DEBUG -D
DYNAMIC_ANNOTATIONS_ENABLED=1 -D WTF_USE_DYNAMIC_ANNOTATIONS=1 -I
../../third_party/class-dump/src/Source -I ../.. -I clang_x64/gen -D __DATE__=
-D __TIME__= -O0 -Wno-builtin-macro-redefined -Wheader-hygiene
-Wstring-conversion -Werror -Wall -Wno-unused-variable -Wpartial-availability
-Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing
-Wno-covered-switch-default -Wno-deprecated-register
-Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override
-Wno-shift-negative-value -Wno-undefined-var-template
-Wno-nonportable-include-path -Wno-address-of-packed-member
-Wno-semicolon-before-method-body -Wno-unused-function -std=c99
-fdebug-compilation-dir
/Users/Shared/Chrome/ios/src/out/gn-Debug-iphonesimulator -ferror-limit 19
-fmessage-length 0 -fvisibility hidden -stack-protector 2 -fblocks
-fobjc-runtime=macosx-10.9.0 -fencode-extended-block-signature -fobjc-arc
-fobjc-exceptions -fexceptions -fmax-type-align=16 -fdiagnostics-show-option
-fcolor-diagnostics -load
../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib
-add-plugin find-bad-constructs -plugin-arg-find-bad-constructs check-templates
-plugin-arg-find-bad-constructs follow-macro-expansion
-plugin-arg-find-bad-constructs enforce-in-pdf -o
clang_x64/obj/third_party/class-dump/class-dump/class-dump.o -x objective-c
../../third_party/class-dump/src/class-dump.m
1. ../../third_party/class-dump/src/class-dump.m:244:71: current parser token
'__DATE__'
2. ../../third_party/class-dump/src/class-dump.m:62:1: parsing function body
'main'
3. ../../third_party/class-dump/src/class-dump.m:62:1: in compound statement
('{}')
4. ../../third_party/class-dump/src/class-dump.m:63:22: in compound statement
('{}')
5. ../../third_party/class-dump/src/class-dump.m:243:33: in compound statement
('{}')
clang: error: unable to execute command: Abort trap: 6
clang: error: clang frontend command failed due to signal (use -v to see
invocation)
clang version 4.0.0 (trunk 278861)
Target: x86_64-apple-darwin15.6.0
Thread model: posix
Message was sent while issue was closed.
justincohen@chromium.org changed reviewers: + justincohen@chromium.org
Message was sent while issue was closed.
The following line in third_party code is causing the failure:
printf("class-dump %s compiled %s\n", CLASS_DUMP_VERSION, __DATE__ " "
__TIME__);
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2255093004/diff/80001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2255093004/diff/80001/build/config/compiler/B... build/config/compiler/BUILD.gn:173: "-Wno-builtin-macro-redefined", This looks pretty hacky. Can we instead teach the compiler real support for this? https://codereview.chromium.org/2255093004/diff/80001/build/config/compiler/B... build/config/compiler/BUILD.gn:174: "-D__DATE__=", __DATE__ and __TIME__ are supposed to be string constants. Instead of defining them to nothing, at least define them to "", which will likely fix the ios bot. But even then, they'll have a weird length, so maybe consider defining them to "Oct 09 2008" and "12:34:56"
Message was sent while issue was closed.
Looking at the assertion message, clang probably has a bug and doesn't expect this, when pchs are used. I think we'll have to revert this for now until that's fixed, but I think this is a good change in general. I'll help getting things unblocked in clang land, and once that's done, we can try again.
Message was sent while issue was closed.
(sent http://lists.llvm.org/pipermail/cfe-dev/2016-August/050516.html to discuss a possibly less hacky approach)
Message was sent while issue was closed.
Filed https://llvm.org/bugs/show_bug.cgi?id=29119 with a reduced repro for the crash (thanks to justincohen for reducing!) I think we'll have to revert this until that bug is fixed.
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2277783002/ by justincohen@chromium.org. The reason for reverting is: This introduces a clang crash when pch are used. See: https://llvm.org/bugs/show_bug.cgi?id=29119.
Message was sent while issue was closed.
I also think we need to wait until the bug fixed. Please feel free to revert (or might already be reverted?)
Message was sent while issue was closed.
On 2016/08/25 03:02:21, Yoshisato Yanagisawa wrote: > I also think we need to wait until the bug fixed. Please feel free to revert > (or might already be reverted?) Wouldn't it be better to define __DATE__ and __TIME__ as a #error macro and eliminate their usage?
Message was sent while issue was closed.
That means you have to change all kinds of third party code, which might be hard. However, if we can pull it off, -Wdate-time can make sure we don't regress. I think not using __DATE__ and __TIME__ in the first place would be best, but my feeling is that it's not practicable. On Aug 25, 2016 2:19 AM, <tansell@chromium.org> wrote: > On 2016/08/25 03:02:21, Yoshisato Yanagisawa wrote: > > I also think we need to wait until the bug fixed. Please feel free to > revert > > (or might already be reverted?) > > Wouldn't it be better to define __DATE__ and __TIME__ as a #error macro and > eliminate their usage? > > https://codereview.chromium.org/2255093004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2016/08/25 14:06:48, Nico wrote: > That means you have to change all kinds of third party code, which might be > hard. However, if we can pull it off, -Wdate-time can make sure we don't > regress. I think not using __DATE__ and __TIME__ in the first place would > be best, but my feeling is that it's not practicable. We had already patched third party code but we didn't ensure that regressions were caught with CI. I'm fine with hardcoding a date but preferably one that is obviously non-sensical, like Jan 1th 1900 at midnight.
Message was sent while issue was closed.
On 2016/08/25 18:47:53, M-A Ruel wrote: > On 2016/08/25 14:06:48, Nico wrote: > > That means you have to change all kinds of third party code, which might be > > hard. However, if we can pull it off, -Wdate-time can make sure we don't > > regress. I think not using __DATE__ and __TIME__ in the first place would > > be best, but my feeling is that it's not practicable. > > We had already patched third party code but we didn't ensure that regressions > were caught with CI. Enable -Wdate-time globally, it'll catch regressions. > > I'm fine with hardcoding a date but preferably one that is obviously > non-sensical, like Jan 1th 1900 at midnight.
Message was sent while issue was closed.
On 2016/08/25 18:50:27, Nico wrote: > Enable -Wdate-time globally, it'll catch regressions. I'll post a CL for this, let's see what happens.
Message was sent while issue was closed.
(looking at -Wdate-time's implementation, it warns on Ident__TIMESTAMP__ too. When this relands, it probably wants to define that macro as well)
Message was sent while issue was closed.
On 2016/08/25 19:08:56, Dirk Pranke wrote: > On 2016/08/25 18:50:27, Nico wrote: > > Enable -Wdate-time globally, it'll catch regressions. > > I'll post a CL for this, let's see what happens. CL: https://codereview.chromium.org/2284453002/
Message was sent while issue was closed.
I think you can land this now. (But set __TIMESTAMP__ as well.) |
