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

Fix conversion on various files #8135

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gasbytes
Copy link
Contributor

@gasbytes gasbytes commented Oct 31, 2024

These ones were linux specific (x86_64 on Void Linux).
Most of them were automated using a shell script that I wrote locally, that uses a vim interactive shell.
Working on the next block of files.

Testing: "gcc (GCC) 13.2.0"

$ ./configure --enable-all CC=gcc 'CFLAGS=-Werror -Wno-pragmas -Wall -Wextra -Wunknown-pragmas --param=ssp-buffer-size=1 -Waddress -Warray-bounds -Wbad-function-cast -Wchar-subscripts -Wcomment -Wfloat-equal -Wformat-security -Wformat=2 -Wmaybe-uninitialized -Wmissing-field-initializers -Wmissing-noreturn -Wmissing-prototypes -Wnested-externs -Wnormalized=id -Woverride-init -Wpointer-arith -Wpointer-sign -Wshadow -Wsign-compare -Wstrict-overflow=1 -Wstrict-prototypes -Wswitch-enum -Wundef -Wunused -Wunused-result -Wunused-variable -Wwrite-strings -fwrapv -Wsign-conversion -fmax-errors=1'
$ make

@gasbytes gasbytes requested a review from douzzer October 31, 2024 17:59
@gasbytes gasbytes assigned wolfSSL-Bot and gasbytes and unassigned wolfSSL-Bot and gasbytes Oct 31, 2024
douzzer
douzzer previously requested changes Nov 9, 2024
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.

rebase+reconciliation needed.

CONFLICT (content): Merge conflict in src/tls.c

looks good otherwise.

@gasbytes
Copy link
Contributor Author

retest this please

@gasbytes gasbytes assigned wolfSSL-Bot and unassigned gasbytes Nov 13, 2024
@dgarske
Copy link
Contributor

dgarske commented Nov 15, 2024

@gasbytes please share how you produced these conversion warnings. Which complier and what build steps. Thank you

@dgarske
Copy link
Contributor

dgarske commented Nov 15, 2024

@gasbytes please squash.

@gasbytes
Copy link
Contributor Author

gasbytes commented Nov 15, 2024

@gasbytes please share how you produced these conversion warnings. Which complier and what build steps. Thank you

@dgarske
Of course, version of gcc:

$ gcc --version
$ gcc (GCC) 13.2.0

And this is the configuration, followed just by a make:

$ ./config.status --config
$ --enable-all CC=gcc 'CFLAGS=-Werror -Wno-pragmas -Wall -Wextra -Wunknown-pragmas --param=ssp-buffer-size=1 -Waddress -Warray-bounds -Wbad-function-cast -Wchar-subscripts -Wcomment -Wfloat-equal -Wformat-security -Wformat=2 -Wmaybe-uninitialized -Wmissing-field-initializers -Wmissing-noreturn -Wmissing-prototypes -Wnested-externs -Wnormalized=id -Woverride-init -Wpointer-arith -Wpointer-sign -Wshadow -Wsign-compare -Wstrict-overflow=1 -Wstrict-prototypes -Wswitch-enum -Wundef -Wunused -Wunused-result -Wunused-variable -Wwrite-strings -fwrapv -Wsign-conversion -fmax-errors=1'

@dgarske
Copy link
Contributor

dgarske commented Nov 15, 2024

Retest this please:

[make check (macos-latest, --enable-harden-tls)](https://github.com/wolfSSL/wolfssl/pull/8135#logs)

Test complete
wolfSSL error: tcp connect failed: Connection refused

Running simple test
SSL version is TLSv1.2
SSL cipher suite is TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
SSL curve name is SECP256R1
SSL version is TLSv1.2
SSL cipher suite is TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
SSL curve name is SECP256R1
Client message: hello wolfssl!
I hear you fa shizzle!

Running TLS test
FAIL testsuite/testsuite.test (exit status: 1)
./configure --enable-aesgcm=table --enable-all --enable-intelasm --enable-sp-math-all 
FAIL scripts/ocsp-stapling_tls13multi.test (exit status: 1)

dgarske
dgarske previously approved these changes Nov 15, 2024
@dgarske dgarske assigned douzzer and unassigned douzzer and gasbytes Nov 15, 2024
@douzzer
Copy link
Contributor

douzzer commented Nov 16, 2024

has a slew of conflicts relative to current master:

CONFLICT (content): Merge conflict in configure.ac
CONFLICT (content): Merge conflict in src/ssl.c
CONFLICT (content): Merge conflict in src/tls.c
CONFLICT (content): Merge conflict in tests/quic.c

it's a mystery to me why github thinks "Merging can be performed automatically"...

@douzzer douzzer assigned gasbytes and unassigned wolfSSL-Bot Nov 16, 2024
@gasbytes
Copy link
Contributor Author

@douzzer

I think this might be because I resolved those conflicts using GitHub's web editor, but I'm not entirely sure why GitHub now says the merge can be performed automatically. As far as I can tell, those files don’t seem to have any breaking changes compared to master, so that might also be a reason.

@gasbytes gasbytes assigned gasbytes and unassigned gasbytes and wolfSSL-Bot Nov 21, 2024
@gasbytes
Copy link
Contributor Author

Working on a couple more Renesas specific.

@gasbytes
Copy link
Contributor Author

gasbytes commented Dec 18, 2024

Retest this please (history lost)

@gasbytes gasbytes assigned wolfSSL-Bot and unassigned gasbytes Dec 19, 2024
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.

needs another round of rebase reconciliation:

CONFLICT (content): Merge conflict in wolfcrypt/src/misc.c
CONFLICT (content): Merge conflict in wolfcrypt/src/port/Renesas/renesas_fspsm_util.c

@douzzer douzzer assigned douzzer and gasbytes and unassigned wolfSSL-Bot Dec 20, 2024
@gasbytes gasbytes force-pushed the fix-conversion branch 2 times, most recently from bb67432 to 9756eac Compare December 21, 2024 00:33
@douzzer
Copy link
Contributor

douzzer commented Dec 21, 2024

retest this please (numerous inexplicable test timeouts)

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.

@gasbytes -- unfortunately, the history on your branch needs to be completely reset, and the changes need to be reapplied manually relative to master.

The problem is that you did a "Merge branch 'master' into fix-conversion", rather than a git rebase master that would have properly preserved the upstream history.

See the git log for the crazy log entry concatenating a hundred or so commit messages and attributing it to @LinuxJedi for reasons unknown. ("200 files changed, 8520 insertions(+), 2959 deletions(-)") That commit is not in the history of master and shouldn't be in it.

Also this branch now has at least one unwanted and unrelated change in it, to configure.ac, and you're going to need to go through the entire change set to make sure there aren't any others.

One way to fix the mess, tested locally:

git checkout master
git pull upstream master
git checkout fix-conversion
git reset --soft master
git diff master configure.ac | git apply --reverse
git commit -n -a

This will create a single new commit with all your changes except for the configure.ac change, directly atop current master, with a perfectly clean history.

As long as that change to configure.ac was the only mistaken one, that'll do the trick.

@gasbytes gasbytes force-pushed the fix-conversion branch 3 times, most recently from 399d293 to c5469d1 Compare December 23, 2024 13:24
@gasbytes
Copy link
Contributor Author

Retest this please

@gasbytes gasbytes assigned wolfSSL-Bot and unassigned douzzer and gasbytes Dec 23, 2024
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.

The branch has many unintended and unwanted changes, all of them in a single commit at the head.

With the branch checked out,

git show --stat HEAD

shows the problem.

 280 files changed, 7111 insertions(+), 8905 deletions(-)

@douzzer douzzer assigned gasbytes and unassigned wolfSSL-Bot Dec 24, 2024
@gasbytes gasbytes force-pushed the fix-conversion branch 2 times, most recently from 8f21d6f to 2b807b5 Compare December 24, 2024 13:02
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.

5 participants