1860646 - GCC compilation failure: this statement may fall through [-Werror=implicit-fallthrough=] comm/mailnews/extensions/smime/nsCMS.cpp

Status: RESOLVED FIXED

(MailNews Core :: Security: S/MIME, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: patch to add GCC warning suprression directive [[fallthrough]]

I am using GCC-13 for local compilation of C-C TB under Debian GNU/Linux.

I get the following error:

/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/extensions/smime/nsCMS.cpp: In member function ‘nsresult nsCMSMessage::CommonVerifySignature(int32_t, const nsTArray<unsigned char>&, int16_t)’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/extensions/smime/nsCMS.cpp:513:7: error: this statement may fall through [-Werror=implicit-fallthrough=]
  513 |       if (verifyFlags & nsICMSVerifyFlags::VERIFY_ALLOW_WEAK_SHA1) {
      |       ^~
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/extensions/smime/nsCMS.cpp:521:5: note: here
  521 |     default:
      |     ^~~~~~~

https://searchfox.org/comm-central/source/mailnews/extensions/smime/nsCMS.cpp#517
shows we have clang directive suppress this warning/error.

So I am attaching a patch for GCC to supress this warning/error.
I am using -Werror to force as many warning as possible during my local build and thus
ignoring this particular warning case is necessary.

Comment 1

[Kai Engert [:KaiE:]]

Dear Chiaki, thank you very much for contributing this fix.

Because it is a simple patch, I was willing to accept it in the provided form, however, I was reminded that it's not the form we can work with easily.

May I suggest that you please use phabricator for submitting future patches?
Everyone involved in reviewing and commit patches on our side has switched to the phabricator tool, which simplifies both review and landing.
Using that tool also allows us to easily have the contributor's name correctly recorded for the commit, and it also allows the contributor to properly set the commit message that is suggested for the patch.

I understand it can be a challenge to switch to different tooling. If I may suggest an idea, you could start by having a separate working directory on your machine, in addition to your usual workflow. This would allow you to experiment with the new environment, without having to abandon your existing setup immediately.

Thank you in advance for considering to help us in this way.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/da5f18174e1d
Suppress C switch fallthrough warning when building with GCC. r=kaie DONTBUILD

Comment 3

[ISHIKAWA, Chiaki]

(In reply to Kai Engert (:KaiE:) from comment #1)

Dear Chiaki, thank you very much for contributing this fix.

Because it is a simple patch, I was willing to accept it in the provided form, however, I was reminded that it's not the form we can work with easily.

May I suggest that you please use phabricator for submitting future patches?
Everyone involved in reviewing and commit patches on our side has switched to the phabricator tool, which simplifies both review and landing.
Using that tool also allows us to easily have the contributor's name correctly recorded for the commit, and it also allows the contributor to properly set the commit message that is suggested for the patch.

I understand it can be a challenge to switch to different tooling. If I may suggest an idea, you could start by having a separate working directory on your machine, in addition to your usual workflow. This would allow you to experiment with the new environment, without having to abandon your existing setup immediately.

Thank you in advance for considering to help us in this way.

Thank you, Kai for your pointers.

Actually, your suggestion of creating a separate repo with new work flow was exactly what I was going to do at the end of August. I had a few summer break days left and I thought I would do that.
Unfortunately, I got infected with Covid-19 then and the long Covid-19 symptoms have not been very friendly.

Only recently I was able to resuming checking and producing of patches of TB and the first thing I needed to do was to make sure that the local tree is in sync with the repository and I can still compile it with my patches.
This was the still the stage yesterday.

I am going to create a new tree for submission as you suggest.
After a few false starts in early August, I figured myself that it would be the most convenient way or gradual switch over while still producing patches already in acceptable format.
Except, I did not create a large VM with large disk image before. (I program under Debian GNU/Linux in my VirtualBox image on a windows PC. A home PC needs to run Windows for various reasons. Thus my setup.)
I am contemplating how I am going to attach a large virtual disk located where. (SSD space is precious on my PC and yet speed is important also.)

Again, thank you for taking care of the patch.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f4b13b5d0bfa
Follow-up fix for clang builds. rs=me