Implement icu4x-key-extract by string tagging #1480

robertbastian · 2022-01-07T14:40:03Z

#948
#1106

jira-pull-request-webhook · 2022-01-10T09:57:05Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

sffc · 2022-01-15T01:33:05Z

This is a clever approach! However, there are two issues:

It ties us to using a macro to generate the ResourceKey, since concat! requires string literals, and we don't have string literals inside const functions. I have long wanted to replace the macro with a const function and I am finally doing it in Re-write ResourceKey #1511.
This approach amounts to duplicating the long form of the ResourceKey string in the executable, which increases code size. It is behind a feature flag, but ideally clients don't need to recompile their app with different settings in order to run the tool.

robertbastian · 2022-01-15T16:20:58Z

The idea for 2 was something like

struct ResourceKey {
  tag_and_human: &'static str,
  pub machine: Something,
}

impl ResourceKey {
  fn human(&self) -> &str {
    &self.tag_and_human[6..]
  }
}

But yes, this requires a macro, but why is that undesirable?

sffc · 2022-01-16T19:56:33Z

We cannot get around the requirement to have a public const function constructor, because macros are codegen at the call site, and they need to be able to call a function in order to construct the ResourceKey. We could still choose to have a macro if we feel that it increases ergonomics, but we need to enforce invariants within the const function.

So, basically, we could potentially make a ResourceKey constructor that looks like

impl ResourceKey {
    /// Returns an error if path_with_tag does not begin with "\nicu4x_key_tag" and does not end with "\n"
    pub const fn try_new(path_with_tag: &str) -> Result<Self, ()> {
        // validate that path_with_tag is in the correct syntax
        // ...
    }
}

And then the macro could use concat! to put together the path_with_tag.

If we go with the approach I've proposed in #1511, the const function takes a human-readable path with no requirement that it also include the tag, and the tag is purely internal to the code with the repr(C) trick.

I do have some worry that if we make this revolve around the human-readable tag, we could end up with false positives. I think false positives are less likely to happen with the repr(C) approach. However, I could be wrong.

jira-pull-request-webhook · 2022-01-19T16:05:06Z

Notice: the branch changed across the force-push!

provider/core/src/hello_world.rs is no longer changed in the branch
provider/core/src/lib.rs is different
provider/core/src/resource.rs is different
provider/core/tests/sizes.rs is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

jira-pull-request-webhook · 2022-01-19T20:41:34Z

Notice: the branch changed across the force-push!

provider/core/src/resource.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

jira-pull-request-webhook · 2022-01-19T22:45:51Z

Notice: the branch changed across the force-push!

tools/scripts/data.toml is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

jira-pull-request-webhook · 2022-01-20T12:53:49Z

Notice: the branch changed across the force-push!

.gitignore is now changed in the branch
Makefile.toml is now changed in the branch
tools/datagen/README.md is now changed in the branch
tools/datagen/src/bin/datagen.rs is now changed in the branch
tools/datagen/src/main.rs is now changed in the branch
tools/datagen/tests/testdata/work_log+keys.txt is now changed in the branch
tools/scripts/data.toml is different
tools/scripts/wasm.toml is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

jira-pull-request-webhook · 2022-01-20T13:06:53Z

Notice: the branch changed across the force-push!

tools/scripts/data.toml is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

robertbastian · 2022-01-20T19:05:41Z

provider/core/src/resource.rs

+        // unsafe {
+        //     // Safe due to invariant that self.path is tagged correctly
+        //     core::str::from_utf8_unchecked(core::slice::from_raw_parts(
+        //         self.path.as_ptr().add(leading_tag!().len()),
+        //         self.path.len() - trailing_tag!().len() - leading_tag!().len(),
+        //     ))
+        // }


Yeah let's go with this

example::ResourceKey::get_path: mov rax, qword ptr [rdi] mov rdx, qword ptr [rdi + 8] add rax, 14 add rdx, -15 ret

For completeness: self.path

example::ResourceKey::get_path: mov rax, qword ptr [rdi] mov rdx, qword ptr [rdi + 8] ret

I think we can live with two extra adds.

robertbastian · 2022-01-20T19:06:28Z

provider/core/src/resource.rs

+        //         self.path.len() - trailing_tag!().len() - leading_tag!().len(),
+        //     ))
+        // }
+        &self.path[leading_tag!().len()..self.path.len() - trailing_tag!().len()]


This is indeed very panicky

example::ResourceKey::get_path: push rax mov rax, qword ptr [rdi] mov rsi, qword ptr [rdi + 8] lea rcx, [rsi - 1] cmp rcx, 14 jb .LBB0_4 cmp rsi, 15 jb .LBB0_4 cmp byte ptr [rax + 14], -64 jl .LBB0_4 cmp byte ptr [rax + rcx], -65 jle .LBB0_4 add rax, 14 add rsi, -15 mov rdx, rsi pop rcx ret .LBB0_4: lea r8, [rip + .L__unnamed_1] mov edx, 14 mov rdi, rax call qword ptr [rip + core::str::slice_error_fail@GOTPCREL] ud2 .L__unnamed_2: .ascii "/app/example.rs" .L__unnamed_1: .quad .L__unnamed_2 .asciz "\017\000\000\000\000\000\000\000\"\000\000\000\n\000\000"

jira-pull-request-webhook · 2022-01-20T20:12:58Z

Notice: the branch changed across the force-push!

tools/datagen/README.md is different
tools/datagen/src/main.rs is different
tools/scripts/data.toml is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

sffc

This PR is missing the actual icu4x-keyextract binary. Do you plan to add it?

provider/core/src/resource.rs

Makefile.toml

provider/core/src/lib.rs

provider/core/src/resource.rs

jira-pull-request-webhook · 2022-01-20T20:38:40Z

Notice: the branch changed across the force-push!

.github/workflows/build-test.yml is now changed in the branch
provider/core/src/lib.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

robertbastian · 2022-01-20T20:40:16Z

The key extract 'binary' is here

jira-pull-request-webhook · 2022-01-20T21:26:35Z

Notice: the branch changed across the force-push!

provider/core/src/resource.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

sffc

Nice work, and thank you for addressing all my concerns!

jira-pull-request-webhook · 2022-01-21T10:57:05Z

Notice: the branch changed across the force-push!

.github/workflows/build-test.yml is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

jira-pull-request-webhook · 2022-01-21T17:08:04Z

Notice: the branch changed across the force-push!

.github/workflows/build-test.yml is no longer changed in the branch
.gitignore is no longer changed in the branch
Makefile.toml is no longer changed in the branch
tools/datagen/README.md is no longer changed in the branch
tools/datagen/src/bin/datagen.rs is no longer changed in the branch
tools/datagen/src/main.rs is no longer changed in the branch
tools/datagen/tests/testdata/work_log+keys.txt is no longer changed in the branch
tools/scripts/wasm.toml is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

provider/core/src/lib.rs

sffc · 2022-01-21T17:15:09Z

tools/scripts/data.toml

-    echo "datetime/symbols@1" >> ${2}
-    echo "plurals/ordinal@1" >> ${2}
-"""
+script = '''strings ${1} | grep -e "icu4x_key_tag\w" | sed s/icu4x_key_tag// | sort > ${2}'''


Thought: Although using strings is handy, we should still offer a cross-platform solution I think. Opinions?

I don't know how complex strings is, i.e. it can probably handle many different binary formats etc. Could be hard to match its sophistication.

Did you verify manually that it works for at least x86_64 (elf) and wasm files?

https://superuser.com/questions/124081/is-there-a-windows-equivalent-of-the-unix-strings-command

I manually verified ELF, and WASM I believe as well, but that's covered by the CI test anyway.

There's this https://docs.microsoft.com/en-us/sysinternals/downloads/strings. The piping, grep, and sed probably won't work on windows though...

It makes me uneasy to think that a pivotal part of the ICU4X data pipeline rests on piping strings to sed. I'd rather have a tool that takes an executable and returns the key list.

(My approval stands; please file a follow up to discuss further)

robertbastian requested a review from sffc January 7, 2022 14:40

robertbastian force-pushed the keys branch from 1e9604b to 9244966 Compare January 10, 2022 09:57

sffc mentioned this pull request Jan 15, 2022

Re-write ResourceKey #1511

Merged

robertbastian force-pushed the keys branch from 1b725d3 to 354ea62 Compare January 19, 2022 16:05

robertbastian force-pushed the keys branch from 2cbb270 to 8ac1f1b Compare January 19, 2022 20:41

robertbastian force-pushed the keys branch from 8ac1f1b to dee4bf2 Compare January 19, 2022 22:45

robertbastian force-pushed the keys branch from dee4bf2 to f7192c8 Compare January 20, 2022 12:53

robertbastian force-pushed the keys branch from f7192c8 to 0fc2b97 Compare January 20, 2022 13:06

robertbastian changed the title ~~Alternative key extract~~ Implement icu4x-key-extract by string tagging Jan 20, 2022

robertbastian commented Jan 20, 2022

View reviewed changes

robertbastian marked this pull request as ready for review January 20, 2022 19:09

robertbastian requested review from Manishearth and a team as code owners January 20, 2022 19:09

robertbastian force-pushed the keys branch from 352d246 to 99abad4 Compare January 20, 2022 20:12

sffc requested changes Jan 20, 2022

View reviewed changes

provider/core/src/resource.rs Outdated Show resolved Hide resolved

Makefile.toml Outdated Show resolved Hide resolved

provider/core/src/lib.rs Outdated Show resolved Hide resolved

provider/core/src/resource.rs Show resolved Hide resolved

robertbastian force-pushed the keys branch from 99abad4 to cb3a9af Compare January 20, 2022 20:38

robertbastian requested a review from sffc January 20, 2022 20:44

robertbastian force-pushed the keys branch from d763f50 to 119c162 Compare January 20, 2022 21:26

sffc previously approved these changes Jan 21, 2022

View reviewed changes

Manishearth previously approved these changes Jan 21, 2022

View reviewed changes

robertbastian dismissed stale reviews from Manishearth and sffc via 6f871e5 January 21, 2022 10:57

robertbastian force-pushed the keys branch from 1173642 to 6f871e5 Compare January 21, 2022 10:57

robertbastian requested review from sffc and Manishearth January 21, 2022 10:57

robertbastian added 7 commits January 21, 2022 18:07

Key extract

314b020

No panic

bc409f8

Reduce prelude

4989255

no overflow panic

2a07b54

merge

a7396ea

doc fix

ac991a1

s/ucu/icu/

e7a72b6

robertbastian force-pushed the keys branch from 6f871e5 to e7a72b6 Compare January 21, 2022 17:08

sffc approved these changes Jan 21, 2022

View reviewed changes

Manishearth removed their request for review January 21, 2022 21:15

robertbastian merged commit 1b159c6 into unicode-org:main Jan 23, 2022

robertbastian mentioned this pull request Jan 23, 2022

Improve icu4x_key_extract #1533

Closed

robertbastian deleted the keys branch January 23, 2022 17:53

gnrunge pushed a commit to gnrunge/icu4x that referenced this pull request Jan 24, 2022

Implement icu4x-key-extract by string tagging (unicode-org#1480)

f23f7cb

This was linked to issues Jan 27, 2022

Implement static data slicing #948

Closed

Build tool for static code analysis for data slicing #1106

Closed

sffc mentioned this pull request Feb 3, 2022

Add icu4x-key-extract for Static Data Slicing #1460

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement icu4x-key-extract by string tagging #1480

Implement icu4x-key-extract by string tagging #1480

robertbastian commented Jan 7, 2022 •

edited

Loading

jira-pull-request-webhook bot commented Jan 10, 2022

sffc commented Jan 15, 2022

robertbastian commented Jan 15, 2022

sffc commented Jan 16, 2022

jira-pull-request-webhook bot commented Jan 19, 2022

jira-pull-request-webhook bot commented Jan 19, 2022

jira-pull-request-webhook bot commented Jan 19, 2022

jira-pull-request-webhook bot commented Jan 20, 2022

jira-pull-request-webhook bot commented Jan 20, 2022

robertbastian Jan 20, 2022

robertbastian Jan 20, 2022

robertbastian Jan 20, 2022

jira-pull-request-webhook bot commented Jan 20, 2022

sffc left a comment

jira-pull-request-webhook bot commented Jan 20, 2022

robertbastian commented Jan 20, 2022

jira-pull-request-webhook bot commented Jan 20, 2022

sffc left a comment

jira-pull-request-webhook bot commented Jan 21, 2022

jira-pull-request-webhook bot commented Jan 21, 2022

sffc Jan 21, 2022

robertbastian Jan 21, 2022

sffc Jan 21, 2022

sffc Jan 21, 2022

sffc Jan 21, 2022

robertbastian Jan 21, 2022

robertbastian Jan 21, 2022

sffc Jan 23, 2022

Implement icu4x-key-extract by string tagging #1480

Implement icu4x-key-extract by string tagging #1480

Conversation

robertbastian commented Jan 7, 2022 • edited Loading

jira-pull-request-webhook bot commented Jan 10, 2022

sffc commented Jan 15, 2022

robertbastian commented Jan 15, 2022

sffc commented Jan 16, 2022

jira-pull-request-webhook bot commented Jan 19, 2022

jira-pull-request-webhook bot commented Jan 19, 2022

jira-pull-request-webhook bot commented Jan 19, 2022

jira-pull-request-webhook bot commented Jan 20, 2022

jira-pull-request-webhook bot commented Jan 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jira-pull-request-webhook bot commented Jan 20, 2022

sffc left a comment

Choose a reason for hiding this comment

jira-pull-request-webhook bot commented Jan 20, 2022

robertbastian commented Jan 20, 2022

jira-pull-request-webhook bot commented Jan 20, 2022

sffc left a comment

Choose a reason for hiding this comment

jira-pull-request-webhook bot commented Jan 21, 2022

jira-pull-request-webhook bot commented Jan 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertbastian commented Jan 7, 2022 •

edited

Loading