Skip to content
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

Remove undef LARGE_HASH_TEST_INPUT_SZ #8049

Closed

Conversation

gojimmypi
Copy link
Contributor

Description

The LARGE_HASH_TEST_INPUT_SZ may be used later in test.c (see line 4130, inside #ifndef NO_UNALIGNED_MEMORY_TEST)

Fixes zd# n/a

Testing

Observed failure and tested only in Espressif.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@gojimmypi gojimmypi requested a review from dgarske October 8, 2024 18:56
@dgarske dgarske assigned wolfSSL-Bot and unassigned dgarske Oct 8, 2024
@dgarske dgarske requested review from douzzer and removed request for dgarske October 8, 2024 19:12
@gojimmypi
Copy link
Contributor Author

Jenkins retest this please

Copy link
Contributor

@douzzer douzzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These LARGE_HASH_TEST_INPUT_SZ definitions are context-specific, defined locally at the start of a span, and undefined when that span is done with it. We currently have two such spans.

The right fix is either to move the #undef later, or add a new #define...#undef span with a value appropriate for that span. They shouldn't be left open.

@gojimmypi
Copy link
Contributor Author

These LARGE_HASH_TEST_INPUT_SZ definitions are context-specific [...] We currently have two such spans.

Any particular reason for that? Why not just have a single setting for the file? I've updated with a proposed fix. Having two same-name macros in two different places with possibly later mismatched values seems to be asking for trouble.

Happy to change it to your suggested, multi-span values if needed

@gojimmypi gojimmypi marked this pull request as draft October 9, 2024 03:38
@gojimmypi
Copy link
Contributor Author

Even though the Jenkins tests all passed, 2/3 of my Espressif tests are now failing.

More investigation needed. Converted to draft. Stand by.

@gojimmypi
Copy link
Contributor Author

@douzzer could you please help me understand why there are still many (word32)sizeof(large_input) instances, along with the two blocks of LARGE_HASH_TEST_INPUT_SZ?

The test.c file is too large to link directly, but there are 10 instances of:

#ifndef NO_LARGE_HASH_TEST
    /* BEGIN LARGE HASH TEST */ {
    byte large_input[1024];

See lines 3165, 3364, 4419, etc.

Should all of these be byte large_input[LARGE_HASH_TEST_INPUT_SZ]; ?

@gojimmypi
Copy link
Contributor Author

ok, all good here. The testing failure was on my dev branch & unrelated. Please see above questions and advise how best I should proceed.

I'm thinking it would be best to make the #define LARGE_HASH_TEST_INPUT_SZ 1024 global in scope and change all the existing instances of byte large_input[1024]; to instead use the defined value.

@gojimmypi gojimmypi marked this pull request as ready for review October 9, 2024 16:03
@douzzer
Copy link
Contributor

douzzer commented Oct 10, 2024

The LARGE_HASH_TEST_INPUT_SZ macros are just local coordinators for bigstack vs smallstack paths, to avoid number literals that might go out of sync in a future moment of inattention. It happens that both neighborhoods where it's defined have it as 1024, but that wasn't necessarily the case and might not stay the case in the future. The macro name and values aren't intended to have any broader meaning or applicability. They were both added earlier this year in 321a72c, which was just a smallstack refactor PR. That refactor, in turn, was guided by -Wframe-larger-than=2048, and it happens that the other instances of byte large_input[1024] didn't (yet) push their associated functions over the 2k limit we observe. We fix these as needed.

@gojimmypi gojimmypi closed this Oct 11, 2024
@gojimmypi gojimmypi force-pushed the pr-fix-LARGE_HASH_TEST_INPUT_SZ branch from de2929f to 9c4960f Compare October 11, 2024 00:13
@gojimmypi
Copy link
Contributor Author

Looks like someone else also encountered this problem & fixed it.

I pushed changes to this branch & retested. All looks well.

@gojimmypi gojimmypi deleted the pr-fix-LARGE_HASH_TEST_INPUT_SZ branch October 11, 2024 01:02
@gojimmypi gojimmypi changed the title remove undef LARGE_HASH_TEST_INPUT_SZ Remove undef LARGE_HASH_TEST_INPUT_SZ Oct 24, 2024
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

Successfully merging this pull request may close these issues.

4 participants