Bug 237779 – [JSC] Nested includes do not change offlineasm hash output

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adrian Perez]

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.

[Adrian Perez]

Xan López found the issue while working on bug #221260 :-)

[Xan Lopez]

(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]

Created attachment 454481 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/90484720>

[Mark Lam]

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]

(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]

Created attachment 456735 [details]
Patch v2

[Mark Lam]

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]

(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]

Created attachment 456790 [details]
Patch for landing

[EWS]

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].