WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
237779
[JSC] Nested includes do not change offlineasm hash output
https://bugs.webkit.org/show_bug.cgi?id=237779
Summary
[JSC] Nested includes do not change offlineasm hash output
Adrian Perez
Reported
2022-03-11 06:34:00 PST
If the offlineasm input for scripts like generate_offset_extractor.rb or generate_settings_extractor.rb results in nested includes, modifying files which are not top-level includes does not regenerate the output. For example, if the top-level file “input.asm” includes “foo.asm”, and that in turn includes “bar.asm”, modifying “bar.asm” and re-running generate_offset_extractor.rb on “input.asm” will exit early without parsing the changed file(s) to update the output file.
Attachments
Patch
(2.78 KB, patch)
2022-03-11 06:48 PST
,
Adrian Perez
no flags
Details
Formatted Diff
Diff
Patch v2
(2.71 KB, patch)
2022-04-05 12:59 PDT
,
Adrian Perez
no flags
Details
Formatted Diff
Diff
Patch for landing
(2.76 KB, patch)
2022-04-06 01:22 PDT
,
Adrian Perez
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Adrian Perez
Comment 1
2022-03-11 06:34:57 PST
Xan López found the issue while working on
bug #221260
:-)
Xan Lopez
Comment 2
2022-03-11 06:36:44 PST
(In reply to Adrian Perez from
comment #0
)
> If the offlineasm input for scripts like generate_offset_extractor.rb or > generate_settings_extractor.rb > results in nested includes, modifying files which are not top-level includes > does not regenerate the > output. > > For example, if the top-level file “input.asm” includes “foo.asm”, and that > in turn includes “bar.asm”, > modifying “bar.asm” and re-running generate_offset_extractor.rb on > “input.asm” will exit early without > parsing the changed file(s) to update the output file.
FWIW I think the code as-is will detect changes to both input.asm and foo.asm, but *not* to bar.asm, because it's already one level too deep. There are no examples of this in the tree right now, but the wasm32 patch includes one (LowLevelInterpreter.asm -> WebAssembly.asm -> WebAssembly32_64.asm, for example]. This patch fixes that situation.
Adrian Perez
Comment 3
2022-03-11 06:48:07 PST
Created
attachment 454481
[details]
Patch
Radar WebKit Bug Importer
Comment 4
2022-03-18 07:34:16 PDT
<
rdar://problem/90484720
>
Mark Lam
Comment 5
2022-03-28 20:09:35 PDT
Comment on
attachment 454481
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=454481&action=review
> Source/JavaScriptCore/offlineasm/parser.rb:884 > + fileList << fileName
I think this is not needed because line 858 above will add it when we recurse into parseIncludes(). Can you confirm by dumping the flattened fileList in parseHash()? I suspect the way you have it now, we're hashing every included file twice.
Adrian Perez
Comment 6
2022-04-05 12:57:21 PDT
(In reply to Mark Lam from
comment #5
)
> Comment on
attachment 454481
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=454481&action=review
> > > Source/JavaScriptCore/offlineasm/parser.rb:884 > > + fileList << fileName > > I think this is not needed because line 858 above will add it when we > recurse into parseIncludes(). Can you confirm by dumping the flattened > fileList in parseHash()? I suspect the way you have it now, we're hashing > every included file twice.
You are right, we don't need to re-add the “fileName” to the list, given that the path was already recorded in the list in line above 858—good catch, thanks! I'll upload a new version of the patch without it.
Adrian Perez
Comment 7
2022-04-05 12:59:45 PDT
Created
attachment 456735
[details]
Patch v2
Mark Lam
Comment 8
2022-04-05 13:14:29 PDT
Comment on
attachment 456735
[details]
Patch v2 View in context:
https://bugs.webkit.org/attachment.cgi?id=456735&action=review
r=me with suggested ChangeLog clarification.
> Source/JavaScriptCore/ChangeLog:13 > + further processed. This adds the missing recursive processing of "include" directives
/processed./processed for 2nd or additional nested levels of includes./ The 1st level is being parsed. "processed" here is a bit ambiguous as in "was it parsed for the hash" or "was it parsed for additional levels of include". I understand that you meant the latter, but rephrasing as above would make it clear.
Adrian Perez
Comment 9
2022-04-06 00:25:11 PDT
(In reply to Mark Lam from
comment #8
)
> Comment on
attachment 456735
[details]
> Patch v2 > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=456735&action=review
> > r=me with suggested ChangeLog clarification. > > > Source/JavaScriptCore/ChangeLog:13 > > + further processed. This adds the missing recursive processing of "include" directives > > /processed./processed for 2nd or additional nested levels of includes./ > > The 1st level is being parsed. "processed" here is a bit ambiguous as in > "was it parsed for the hash" or "was it parsed for additional levels of > include". I understand that you meant the latter, but rephrasing as above > would make it clear.
Yes, you understood correctly. I'll write down the ChangeLog entry with the suggested wording before landing, which makes the intention of the patch clearer. Thanks!
Adrian Perez
Comment 10
2022-04-06 01:22:20 PDT
Created
attachment 456790
[details]
Patch for landing
EWS
Comment 11
2022-04-06 02:06:11 PDT
Committed
r292454
(
249305@main
): <
https://commits.webkit.org/249305@main
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 456790
[details]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug