On Thu, 16 Nov 2023 04:40:53 GMT, Julian Waters <jwat...@openjdk.org> wrote:
>> I regret not actually addressing the issues with the goto labels in >> https://github.com/openjdk/jdk/pull/15996, where initialization of locals in >> sspi were jumped over by gotos to a certain label. I changed the >> initializations into split declarations and assignments in >> https://github.com/openjdk/jdk/pull/15996, but this is simply a hack and >> does not address the real issue of gotos jumping over locals. I've as such >> fixed the issues with them properly this time, by simply deleting the labels >> and duplicating the code where they're used. As mentioned, this >> unfortunately does increase duplicate code, but is the cleanest solution I >> could come up with for the labels > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > NULL to nullptr in sspi.cpp > > > > Can you please describe the problem you are trying to solve, and why > > > > you think it is worth solving. You describe the thought process you > > > > went through while doing the patch and possibly some other, older > > > > patch. As it is, reading the PR text or the issue description is > > > > confusing and requires reviewers to dig through comment threads on > > > > older issues to parse it. > > > > > > > > > I've changed to description for both to (hopefully) be more descriptive > > > and clear. Sorry for the hassle, I'm not particularly good with words > > > > > > Somewhat better, but not by much, sorry :-) > > This patch is work to review, since it touches security-relevant stuff and > > because mentally verifying that all the code paths you change are > > equivalent to what happened before is real work. It is probably more work > > than doing the patch. > > Hence my question, what is the motivation, is there a bug that needs > > fixing? Or is it just that you dislike these gotos? Is it just to get > > Windows to build with GCC? Sorry, I did not follow your work, so I don't > > have a context. > > This is a cleanup patch that follows the older one that enabled security to > compile with permissive-, the problem with that one is that simply changing > the locals to a split declaration and assignment is a bit of an ugly hack, > since the locals then have undefined values when allocated. gcc already > compiles fine with the old patch, this new one is not needed for either > compiler (MSVC or gcc) to compile without errors. I felt like I had the > responsibility to clean up what is essentially a hack just to get it to > compile with permissive- on MSVC. This is also partially inspired by Phil's > rejection of the old patch in #15096 :/ > > I don't have anything against gotos, nor do I dislike them :) Thank you for your explanation! I'll relegate to Phil for the actual review then; I was just curious about the patch since it touched security-related code. Cheers, Thomas ------------- PR Comment: https://git.openjdk.org/jdk/pull/16682#issuecomment-1816661606
