Zucchini: Encode ExecutableType values as ints that encode text. |
||
Issue descriptionAs of 2018/04/19, ExecutableType enums are: kExeTypeNoOp = 0, kExeTypeWin32X86 = 1, kExeTypeWin32X64 = 2, kExeTypeElfX86 = 3, kExeTypeElfX64 = 4, kExeTypeElfArm32 = 5, kExeTypeElfAArch64 = 6, kExeTypeDex = 7, This is arbitrary. Meanwhile in bug 834094 we want to add a new "Disassembler Mock" type ( bug 834904 ). If we want regular Zucchini to use this it makes sense to have this as 1, but this would bumps the other values! We propose to reassign the enums (which are already stored as uint32 in patch) as values that encode integer. For example, kExeTypeWin32X86 can be encoded as 0x36387850U, which in little endian int is [0x50, 0x78, 0x38, 0x36], or "Px86" (without terminating null). These values are much more stable, and also more readable if we have to examine the patch.
,
Apr 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/da4335f0d27c7fa14f6897ffeb0833d424860f7e commit da4335f0d27c7fa14f6897ffeb0833d424860f7e Author: Calder Kitagawa <ckitagawa@google.com> Date: Tue Apr 24 13:54:46 2018 [Zucchini] Update ExecutableType values. This change has some pros and some cons: Pros: - Human readable ExecType in patch/hex dumps as the ASCII is meaningful. - Can be done at next to no cost at compile time. - Assigning new values regardless of order of appearance in the enum is more flexible. No more concern over invalidating old patches as values will be more or less permanent once this change goes in. Cons: - Checking if an ExecutableType is valid requires O(n) time where n is the number of supported Exec types using a cast. Alternatively, we could maintain a sorted list of these types in memory to check against in O(log(n)) or could use a set but this is more memory. Overall not a huge deal since we only support ~9 types but it is worth understanding the tradeoffs. - Enums don't enforce value reuse so it is possible although highly unlikely we introduce a repeated value. However, given the use of the switch case casting requiring unique values this is very unlikely. Bug: 834932 Change-Id: I7bc14ea7b4434e60bb0dafa47178fb2c2c12dc7f Reviewed-on: https://chromium-review.googlesource.com/1020446 Commit-Queue: Calder Kitagawa <ckitagawa@google.com> Reviewed-by: Samuel Huang <huangs@chromium.org> Cr-Commit-Position: refs/heads/master@{#553083} [modify] https://crrev.com/da4335f0d27c7fa14f6897ffeb0833d424860f7e/chrome/test/data/installer/zucchini_archive.diff [modify] https://crrev.com/da4335f0d27c7fa14f6897ffeb0833d424860f7e/components/zucchini/element_detection_unittest.cc [modify] https://crrev.com/da4335f0d27c7fa14f6897ffeb0833d424860f7e/components/zucchini/heuristic_ensemble_matcher.cc [modify] https://crrev.com/da4335f0d27c7fa14f6897ffeb0833d424860f7e/components/zucchini/image_utils.h [modify] https://crrev.com/da4335f0d27c7fa14f6897ffeb0833d424860f7e/components/zucchini/image_utils_unittest.cc [modify] https://crrev.com/da4335f0d27c7fa14f6897ffeb0833d424860f7e/components/zucchini/patch_read_write_unittest.cc [modify] https://crrev.com/da4335f0d27c7fa14f6897ffeb0833d424860f7e/components/zucchini/patch_reader.cc
,
Apr 24 2018
|
||
►
Sign in to add a comment |
||
Comment 1 by huangs@google.com
, Apr 19 2018