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

General project updates; rebase onto upstream v6.11; add notes about CONFIG_WERROR #17

Open
wants to merge 9 commits into
base: llvm-trunk-next
Choose a base branch
from

Conversation

whentojump
Copy link
Member

No description provided.

@@ -1,7 +1,7 @@
From 7c60851daae39815fa696fc2265e838b1913f7ac Mon Sep 17 00:00:00 2001
From: Wentao Zhang <[email protected]>
Copy link
Member Author

@whentojump whentojump Sep 5, 2024

Choose a reason for hiding this comment

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

In the sent version 2 I accidentally included my personal email here in the cover letter. I dry-run with myself a few times, checked a few places but somehow ignored this one..

I suppose it should not create a "Signed-off-by chain" issue as it's not a patch? I will pay attention in future updates.

@whentojump whentojump changed the title General project updates General project updates, rebase onto upstream v6.11-rc7 Sep 12, 2024
@chuckwolber
Copy link
Collaborator

It does not look like any lines of code are changed as a result of the rebase and the v2.0 patches still apply cleanly to v6.11 as well as the current torvalds HEAD commit (fc1dc0d50780). I would not trigger a v3.0 patch set as a result. As long as the patches apply cleanly to the current torvalds HEAD, you do not need to work on a revision.

I recommend you remove the v3.0 patch set and associated changed and limit this PR to project updates.

@chuckwolber
Copy link
Collaborator

chuckwolber commented Sep 24, 2024

This is probably a silly mistake I am making, but since there is possible build breakage, I am erring on the side of caution and reporting early.

Currently investigating an issue where build warnings are suddenly being interpreted as errors when using the latest LLVM 20 HEAD commit. I have an old build of LLVM 20 based on commit b076f6640e3c2781410588f4a8e4ccfeed8eb606 that does not seem to have the issue. I did a full LLVM 20 build from 0673642cab6b6a9eec20d4ea4ee6bc46db47e04c and kernel build warnings seem to be interpreted as errors. I also note that a big pile of LLVM commits just landed after 0673642cab6b6a9eec20d4ea4ee6bc46db47e04c, which somewhat complicates the issue.

I am going to re-verify LLVM 19.1.0 and bisect the LLVM 20 build. Either I will find a specific commit that is causing the issue, or figure out the silly mistake I am making.

llvm-project bisection progress:

Bad:  9830156f623c56062bf6df1b4c4b4bd8ab5bd57c
Bad:  0673642cab6b6a9eec20d4ea4ee6bc46db47e04c
Bad:  9cd93774098c861c260090a690f428b7ae031c65
Bad:  2fb133f32af42b29e1e1bbbbc24fb75eea3f34a2
Good: 069b841c2ecde39c65ca74660b681d4d25a4bbb2
Good: cd392420322f9968f76cd7f4afb6726d5912b236
Good: cd774c873c891776403ada6962d3f1602efd4054
Good: 159e5b3fdf26325403ad3756d43e625b7d59443a
Good: 89c10e27d8b4d5f44998aad9abd2590d9f96c5df
Good: 35e27c0ee51f2822415c050c1cc4a73dfaa171d7
Good: e25eb1433110d94d16fd69e5aca9bdf72259263d
Good: b076f6640e3c2781410588f4a8e4ccfeed8eb606

@whentojump
Copy link
Member Author

whentojump commented Sep 24, 2024

Thanks for the investigation. What are the build warnings/errors you are seeing? Are they those MC/DC related ones? Can you post it here? Update 1: Ok. I'm reproducing it as well.

I can also take a look.


Update 2: the root cause is, previously -Werror wouldn't turn MC/DC warnings into errors and now it does.

Check the following with your old and new LLVM respectively.

cat > test.c << 'EOF'
int foo(int x) { return x; }
int main(void) {
    int a, b, c, d, e, f, g, h;
    a && b && c && d && e && f && g;
    a && foo( b && c );
    return 0;
}
EOF
clang -Xclang -fmcdc-max-conditions=6 -Werror -Wno-unused-value -fprofile-instr-generate -fcoverage-mapping -fcoverage-mcdc test.c -o test

Update 3: I can confirm llvm/llvm-project@e7f782e is one of the breaking commits. By "one of" I mean the PR in question was applied and reverted multiple times that day (history), so it's possible to see pattern "...warning, error, warning, error..." over that period of time. Added a note in LLVM community here.

For our patch, I suppose the only solution in the short term is to disable CONFIG_WERROR.

@chuckwolber
Copy link
Collaborator

I guess the good news is that this fixes some buggy Clang behavior. I tested LLVM 19.1 and it still displays the old behavior, so the patches should work just fine for anyone testing with a release version of LLVM.

I think I agree that CONFIG_WERROR might be a good option for clang 20+, but I do not think it makes sense to automatically turn off CONFIG_WERROR if MC/DC is turned on and CLANG_VERSION >= 200000. I think it would cause confusion and probably break stuff later on.

Looking at the LLVM code, these look like custom diagnostics here and here, which means we are not likely to find a diagnostic flag flag that we can disable with a -Wno-<flag>.

We might be able to narrowly manage this if the llvm-project is willing to let us add some diagnostic flags for split nested logical ops and MC/DC max conditions. Then we can update the kernel patch to only disable those particular diagnostics (e.g. -Wno-split-nested -Wno-mcdc-max-conditions) when MC/DC is turned on, and avoid the need to turn off CONFIG_WERROR. This seems like an iffy approach though, since I would imagine someone is going to fix both of those issues at some point, which means there will not be any need to handle those warnings.

Probably the best approach is to add some text in the Kconfig help section warning about this issue. Maybe a lot of people will miss it, but at least we can say we put it in there. We could include that with a v3 patch set now that mainline has some changes that cause the v2 patches to not apply cleanly.

I can also confirm using your example:

# llvm-project commit b076f6640e3c2781410588f4a8e4ccfeed8eb606
clang -Xclang -fmcdc-max-conditions=6 -Werror -Wno-unused-value -fprofile-instr-generate -fcoverage-mapping -fcoverage-mcdc test.c -o test
test.c:4:5: warning: unsupported MC/DC boolean expression; number of conditions (7) exceeds max (6). Expression will not be covered
    4 |     a && b && c && d && e && f && g;
      |     ^
test.c:5:5: warning: unsupported MC/DC boolean expression; contains an operation with a nested boolean expression. Expression will not be covered
    5 |     a && foo( b && c );
      |     ^
2 warnings generated.
# llvm-project commit 9830156f623c56062bf6df1b4c4b4bd8ab5bd57c
clang -Xclang -fmcdc-max-conditions=6 -Werror -Wno-unused-value -fprofile-instr-generate -fcoverage-mapping -fcoverage-mcdc test.c -o test
test.c:4:5: error: unsupported MC/DC boolean expression; number of conditions (7) exceeds max (6). Expression will not be covered [-Werror]
    4 |     a && b && c && d && e && f && g;
      |     ^
test.c:5:5: error: unsupported MC/DC boolean expression; contains an operation with a nested boolean expression. Expression will not be covered [-Werror]
    5 |     a && foo( b && c );
      |     ^
2 errors generated.

@whentojump whentojump changed the title General project updates, rebase onto upstream v6.11-rc7 General project updates; rebase onto upstream v6.11; add notes about CONFIG_WERROR Sep 25, 2024
* No longer keep "tentative repository structure" as most of the
  important stuffs have already been referenced elsewhere in README
* Add YouTube live links with anchors before the split version videos
  are released
@whentojump
Copy link
Member Author

Let me resolve Nathan's comments and rebase onto 6.12-rc1 in a day

@whentojump
Copy link
Member Author

Nathan's first round of comments have been resolved or responded. See patches/v3.0 in this PR and my replies in the mailing list.

In the PR, I have reverted the earlier bit about CONFIG_WERROR since that -Werror behavior is quite unexpected by many other Clang components and thus got reverted. Let's maybe wait for the dust to settle in LLVM upstream. (This is also somewhat related to Nathan's comments on suppressing the MC/DC warnings.)

The booting issue reported by Nathan in his latest reply can be reproduced on my side. I will investigate that.

Another thing I noticed with this series is there is no entries added to
MAINTAINERS. Who is going to be responsible for maintaining this code?

I think we can put Chuck and me there. @chuckwolber you mentioned in another comment you can do that, which is appreciated! Please let us know if there's any other consideration e.g. clearance from Boeing.

@chuckwolber
Copy link
Collaborator

Nathan's first round of comments have been resolved or responded. See patches/v3.0 in this PR and my replies in the mailing list.

In the PR, I have reverted the earlier bit about CONFIG_WERROR since that -Werror behavior is quite unexpected by many other Clang components and thus got reverted. Let's maybe wait for the dust to settle in LLVM upstream. (This is also somewhat related to Nathan's comments on suppressing the MC/DC warnings.)

Agreed. Also, I believe since the nested boolean warning is due to unimplemented functionality in the LLVM MC/DC code, we should expect that that error will eventually go away.

The booting issue reported by Nathan in his latest reply can be reproduced on my side. I will investigate that.

Also investigating it myself and I am able to reproduce the problem as well. The objtool warnings may indicate that this is a simple matter of adding additional noinstr instructions.

Another thing I noticed with this series is there is no entries added to
MAINTAINERS. Who is going to be responsible for maintaining this code?

I think we can put Chuck and me there. @chuckwolber you mentioned in another comment you can do that, which is appreciated! Please let us know if there's any other consideration e.g. clearance from Boeing.

Yes, that is correct, and no additional approvals or clearances required. I think we can add a fifth patch to the v3 set to update the MAINTAINERS file once it is ready.

Also, please add my personal email address ([email protected] ) to the list of patch recipients starting with the v3 patch set once they are ready to be sent.

@whentojump
Copy link
Member Author

@chuckwolber Please take a look at the latest patches/v3 in this PR. Do you suggest we send it out before or after we have a better understanding of the boot issue?

@chuckwolber
Copy link
Collaborator

@chuckwolber Please take a look at the latest patches/v3 in this PR. Do you suggest we send it out before or after we have a better understanding of the boot issue?

I have been testing them. Other than the boot hang issue, they look good. I still need to do a close review of the Kconfig wording changes though.

Yes, I think we should get a better understanding of the boot issue before we submit v3. I was not able to get to it today, but my plan is to add noinstr to quiet down the objtool warnings. If that solves the problem, we can talk further about potential solutions.

@chuckwolber
Copy link
Collaborator

chuckwolber commented Oct 15, 2024

I believe I can confirm CONFIG_FORTIFY_SOURCE triggers the hang when the kernel is instrumented for LLVM code coverage. The following trivial proof-of-concept patches allow us to build and run without hanging when CONFIG_FORTIFY_SOURCE is turned on. Without these trivial patches, I built with GCOV instrumentation (using GCC of course) and was not able to see a similar hang.

I am going to drill down on the fortify-strings.h code to see if there is a specific function that triggers the hang.

diff --git a/include/linux/string.h b/include/linux/string.h
index 0dd27afcfaf7..c7c8821fdae6 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -386,9 +386,9 @@ static inline const char *kbasename(const char *path)
        return tail ? tail + 1 : path;
 }

-#if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE)
-#include <linux/fortify-string.h>
-#endif
+//#if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE)
+//#include <linux/fortify-string.h>
+//#endif
 #ifndef unsafe_memcpy
 #define unsafe_memcpy(dst, src, bytes, justification)          \
        memcpy(dst, src, bytes)
diff --git a/kernel/Makefile b/kernel/Makefile
index 87866b037fbe..ea63bec021f2 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -116,6 +116,7 @@ obj-$(CONFIG_SHADOW_CALL_STACK) += scs.o
 obj-$(CONFIG_HAVE_STATIC_CALL) += static_call.o
 obj-$(CONFIG_HAVE_STATIC_CALL_INLINE) += static_call_inline.o
 obj-$(CONFIG_CFI_CLANG) += cfi.o
+obj-$(CONFIG_LLVM_COV_KERNEL) += llvm-cov/

 obj-$(CONFIG_PERF_EVENTS) += events/

diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 4f887aa62fa0..01295839dfa5 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -1009,35 +1009,35 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
 }
 EXPORT_SYMBOL(memcpy_and_pad);

-#ifdef CONFIG_FORTIFY_SOURCE
-/* These are placeholders for fortify compile-time warnings. */
-void __read_overflow2_field(size_t avail, size_t wanted) { }
-EXPORT_SYMBOL(__read_overflow2_field);
-void __write_overflow_field(size_t avail, size_t wanted) { }
-EXPORT_SYMBOL(__write_overflow_field);
-
-static const char * const fortify_func_name[] = {
-#define MAKE_FORTIFY_FUNC_NAME(func)   [MAKE_FORTIFY_FUNC(func)] = #func
-       EACH_FORTIFY_FUNC(MAKE_FORTIFY_FUNC_NAME)
-#undef  MAKE_FORTIFY_FUNC_NAME
-};
-
-void __fortify_report(const u8 reason, const size_t avail, const size_t size)
-{
-       const u8 func = FORTIFY_REASON_FUNC(reason);
-       const bool write = FORTIFY_REASON_DIR(reason);
-       const char *name;
-
-       name = fortify_func_name[umin(func, FORTIFY_FUNC_UNKNOWN)];
-       WARN(1, "%s: detected buffer overflow: %zu byte %s of buffer size %zu\n",
-                name, size, str_read_write(!write), avail);
-}
-EXPORT_SYMBOL(__fortify_report);
-
-void __fortify_panic(const u8 reason, const size_t avail, const size_t size)
-{
-       __fortify_report(reason, avail, size);
-       BUG();
-}
-EXPORT_SYMBOL(__fortify_panic);
-#endif /* CONFIG_FORTIFY_SOURCE */
+//#ifdef CONFIG_FORTIFY_SOURCE
+///* These are placeholders for fortify compile-time warnings. */
+//void __read_overflow2_field(size_t avail, size_t wanted) { }
+//EXPORT_SYMBOL(__read_overflow2_field);
+//void __write_overflow_field(size_t avail, size_t wanted) { }
+//EXPORT_SYMBOL(__write_overflow_field);
+//
+//static const char * const fortify_func_name[] = {
+//#define MAKE_FORTIFY_FUNC_NAME(func) [MAKE_FORTIFY_FUNC(func)] = #func
+//     EACH_FORTIFY_FUNC(MAKE_FORTIFY_FUNC_NAME)
+//#undef  MAKE_FORTIFY_FUNC_NAME
+//};
+//
+//void __fortify_report(const u8 reason, const size_t avail, const size_t size)
+//{
+//     const u8 func = FORTIFY_REASON_FUNC(reason);
+//     const bool write = FORTIFY_REASON_DIR(reason);
+//     const char *name;
+//
+//     name = fortify_func_name[umin(func, FORTIFY_FUNC_UNKNOWN)];
+//     WARN(1, "%s: detected buffer overflow: %zu byte %s of buffer size %zu\n",
+//              name, size, str_read_write(!write), avail);
+//}
+//EXPORT_SYMBOL(__fortify_report);
+//
+//void __fortify_panic(const u8 reason, const size_t avail, const size_t size)
+//{
+//     __fortify_report(reason, avail, size);
+//     BUG();
+//}
+//EXPORT_SYMBOL(__fortify_panic);
+//#endif /* CONFIG_FORTIFY_SOURCE */

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.

3 participants