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.
Xan López found the issue while working on bug #221260 :-)
(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.
Created attachment 454481 [details] Patch
<rdar://problem/90484720>
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.
(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.
Created attachment 456735 [details] Patch v2
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.
(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!
Created attachment 456790 [details] Patch for landing
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].