Skip to content

Large code size impact when using sha1 functions #6568

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
5 of 6 tasks
dirkmueller opened this issue Sep 29, 2019 · 1 comment
Closed
5 of 6 tasks

Large code size impact when using sha1 functions #6568

dirkmueller opened this issue Sep 29, 2019 · 1 comment

Comments

@dirkmueller
Copy link
Contributor

Basic Infos

  • This issue complies with the issue POLICY doc.
  • I have read the documentation at readthedocs and the issue is not addressed there.
  • I have tested that the issue is present in current master branch (aka latest git).
  • I have searched the issue tracker for a similar issue.
  • If there is a stack dump, I have decoded it.
  • I have filled out all fields below.

Platform

  • Hardware: [ESP-12]
  • Core Version: 2.5.2
  • Development Env: Platformio
  • Operating System: MacOS

Problem Description

Using this code in a sketch:

  String s = "abc";
  return sha1(s);

causes the resulting firmware bin to grow by about 6kb of code size. this is predominantly due to the function SHA1Transform:

1076073460 00000269 T SHA1_Final
1076072872 00000467 t SHA1ProcessMessageBlock
1076153740 00004595 T SHA1Transform

The root cause for that is ./libraries/Hash/src/sha1/sha1.c which is a large-machine optimized SHA1 implementation using unrolled loops and other optimisations for speed that are likely extremely unuseful for esp8266.

Interestingly in a typical sketch build there is also a 2nd sha1 implementation (at least) included from bearssl, which provides a similar api but with a much smaller footprint:

1076042492 00000049 T br_sha1_init
1076042440 00000051 T br_sha1_set_state
1076043468 00000123 T br_sha1_update
1076043592 00000235 T br_sha1_out
1076042584 00000881 T br_sha1_round

A very naive replacement of the lib/sha1 with the corresponding implementation from bearssl gives the expected footprint (code size) reduction, but it isn't clear to me if that is acceptable policy wise (dependency on bearssl in core hash lib). the impact in typical Arduino sketch builds using sha1() and https related functions is that we have (at least) two sha1 implemenations in the resulting binary, and one of them being very excessive in size.

@earlephilhower
Copy link
Collaborator

We already use BearSSL internal things in the Updater class (for signing), so I think a PR with your naive implementation would be fine. Just please make sure the API is the same as existing one to make it a transparent swap.

dirkmueller added a commit to dirkmueller/Arduino that referenced this issue Sep 30, 2019
The Hash library had its own copy of a loop-unrolled sha1 implementation
adding a large code footprint for no good reason, as there are several
sha1 implementations already in tree (one in NONOS-SDK as well as one
in bearssl). Switching to the bearssl one is straightforward and removes
about 3kb of code size overhead.

Also cleanup some obvious inefficiencies (copy by value, string
summing, no reservation causing repeated reallocations) in the
implementation.
earlephilhower added a commit to earlephilhower/Arduino that referenced this issue Sep 30, 2019
As @DirkMuller found out in esp8266#6568, there is a difference in code
executed between `String str(nullptr)` and `String str("")`, but in the
end the actual object is identical.  It's a few bytes of code, but every
little bit counts.

Update the default `String()` constructor to use `nullptr` and not `""`.
This will remove a constant literal load and the execution of the
String::copy method and strlen().
devyte pushed a commit that referenced this issue Oct 1, 2019
As @DirkMuller found out in #6568, there is a difference in code
executed between `String str(nullptr)` and `String str("")`, but in the
end the actual object is identical.  It's a few bytes of code, but every
little bit counts.

Update the default `String()` constructor to use `nullptr` and not `""`.
This will remove a constant literal load and the execution of the
String::copy method and strlen().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants