Similar functions in net/core/dev.c
Hi, All these functions in net/core/dev.c are very similar. __netdev_walk_all_upper_dev netdev_walk_all_upper_dev_rcu netdev_walk_all_lower_dev __netdev_walk_all_lower_dev netdev_walk_all_lower_dev_rcu Can they be merged in one function? Are they a hot path? Regards, Vinicius
Re: [PATCH v3] ext4: Enable code path when DX_DEBUG is set
On Wed, Feb 3, 2021 at 2:39 AM Theodore Ts'o wrote:
>
> On Tue, Feb 02, 2021 at 04:28:37PM +, Vinicius Tinti wrote:
> > Clang with -Wunreachable-code-aggressive is being used to try to find
> > unreachable code that could cause potential bugs. There is no plan to
> > enable it by default.
> >
> > The following code was detected as unreachable:
> >
> > fs/ext4/namei.c:831:17: warning: code will never be executed
> > [-Wunreachable-code]
> > unsigned n = count - 1;
> > ^
> > fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as
> > explicitly dead
> > if (0) { // linear search cross check
> > ^
> > /* DISABLES CODE */ ( )
> >
> > This has been present since commit ac27a0ec112a ("[PATCH] ext4: initial
> > copy of files from ext3") and fs/ext3 had it present at the beginning of
> > git history. It has not been changed since.
> >
> > This patch moves the code to a new function `htree_rep_invariant_check`
> > which only performs the check when DX_DEBUG is set. This allows the
> > function to be used in other parts of the code.
> >
> > Suggestions from: Andreas, Christoph, Nathan, Nick and Ted.
> >
> > Signed-off-by: Vinicius Tinti
>
> Thanks, applied, although I rewrote the commit description:
>
> ext4: factor out htree rep invariant check
>
> This patch moves some debugging code which is used to validate the
> hash tree node when doing a binary search of an htree node into a
> separate function, which is disabled by default (since it is only used
> by developers when they are modifying the htree code paths).
>
> In addition to cleaning up the code to make it more maintainable, it
> silences a Clang compiler warning when -Wunreachable-code-aggressive
> is enabled. (There is no plan to enable this warning by default, since
> there it has far too many false positives; nevertheless, this commit
> reduces one of the many false positives by one.)
>
> The original description buried the lede, in terms of the primary
> reason why I think the change was worthwhile (although I know you have
> different priorities than mine :-).
>
> Thanks for working to find a way to improve the code in a way that
> makes both of us happy!
Thanks for the feedback.
And thanks for all the ones who reviewed.
> - Ted
[PATCH v3] ext4: Enable code path when DX_DEBUG is set
Clang with -Wunreachable-code-aggressive is being used to try to find
unreachable code that could cause potential bugs. There is no plan to
enable it by default.
The following code was detected as unreachable:
fs/ext4/namei.c:831:17: warning: code will never be executed
[-Wunreachable-code]
unsigned n = count - 1;
^
fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as
explicitly dead
if (0) { // linear search cross check
^
/* DISABLES CODE */ ( )
This has been present since commit ac27a0ec112a ("[PATCH] ext4: initial
copy of files from ext3") and fs/ext3 had it present at the beginning of
git history. It has not been changed since.
This patch moves the code to a new function `htree_rep_invariant_check`
which only performs the check when DX_DEBUG is set. This allows the
function to be used in other parts of the code.
Suggestions from: Andreas, Christoph, Nathan, Nick and Ted.
Signed-off-by: Vinicius Tinti
---
fs/ext4/namei.c | 38 --
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index cf652ba3e74d..a6e28b4b5a95 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -731,6 +731,29 @@ struct stats dx_show_entries(struct dx_hash_info *hinfo,
struct inode *dir,
(space/bcount)*100/blocksize);
return (struct stats) { names, space, bcount};
}
+
+/*
+ * Linear search cross check
+ */
+static inline void htree_rep_invariant_check(struct dx_entry *at,
+struct dx_entry *target,
+u32 hash, unsigned int n)
+{
+ while (n--) {
+ dxtrace(printk(KERN_CONT ","));
+ if (dx_get_hash(++at) > hash) {
+ at--;
+ break;
+ }
+ }
+ ASSERT(at == target - 1);
+}
+#else /* DX_DEBUG */
+static inline void htree_rep_invariant_check(struct dx_entry *at,
+struct dx_entry *target,
+u32 hash, unsigned int n)
+{
+}
#endif /* DX_DEBUG */
/*
@@ -827,20 +850,7 @@ dx_probe(struct ext4_filename *fname, struct inode *dir,
p = m + 1;
}
- if (0) { // linear search cross check
- unsigned n = count - 1;
- at = entries;
- while (n--)
- {
- dxtrace(printk(KERN_CONT ","));
- if (dx_get_hash(++at) > hash)
- {
- at--;
- break;
- }
- }
- ASSERT(at == p - 1);
- }
+ htree_rep_invariant_check(entries, p, hash, count - 1);
at = p - 1;
dxtrace(printk(KERN_CONT " %x->%u\n",
--
2.25.1
Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set
On Mon, Feb 1, 2021 at 6:41 PM Nick Desaulniers wrote:
>
> On Mon, Feb 1, 2021 at 1:38 PM Theodore Ts'o wrote:
> >
> > On Mon, Feb 01, 2021 at 01:16:19PM -0800, Nick Desaulniers wrote:
> > > I agree; Vinicius, my recommendation for -Wunreachable-* with Clang
> > > was to see whether dead code identified by this more aggressive
> > > diagnostic (than -Wunused-function) was to ask maintainers whether
> > > code identified by it was intentionally dead and if they would
> > > consider removing it. If they say "no," that's fine, and doesn't need
> > > to be pushed. It's not clear to maintainers that:
> > > 1. this warning is not on by default
> > > 2. we're not looking to pursue turning this on by default
Ok. I will make it clear in next commit messages.
> > >
> > > If maintainers want to keep the dead code, that's fine, let them and
> > > move on to the next instance to see if that's interesting (or not).
> >
> > It should be noted that in Documenting/process/coding-style.rst, there
> > is an expicit recommendation to code in a way that will result in dead
> > code warnings:
> >
> >Within code, where possible, use the IS_ENABLED macro to convert a
> > Kconfig
> >symbol into a C boolean expression, and use it in a normal C conditional:
> >
> >.. code-block:: c
> >
> > if (IS_ENABLED(CONFIG_SOMETHING)) {
> > ...
> > }
> >
> >The compiler will constant-fold the conditional away, and include or
> > exclude
> >the block of code just as with an #ifdef, so this will not add any
> > runtime
> >overhead. However, this approach still allows the C compiler to see the
> > code
> >inside the block, and check it for correctness (syntax, types, symbol
> >references, etc). Thus, you still have to use an #ifdef if the code
> > inside the
> >block references symbols that will not exist if the condition is not met.
> >
> > So our process documentation *explicitly* recommends against using
> > #ifdef CONFIG_XXX ... #endif, and instead use something that will
> > -Wunreachable-code-aggressive to cause the compiler to complain.
>
> I agree.
I agree too.
> >
> > Hence, this is not a warning that we will *ever* be able to enable
> > unconditionally ---
>
> I agree.
>
> > so why work hard to remove such warnings from the
> > code? If the goal is to see if we can detect real bugs using this
>
> Because not every instance of -Wunreachable-code-aggressive may be that
> pattern.
The goal is to try to detect real bugs. In this instance specifically I
suggested to remove the "if (0) {...}" because it sounded like an
unused code.
If it is useful it is fine to keep.
For now I am only looking for dead code that cannot be enabled
by a configuration file or architecture. In fact, there are several
warnings that I am ignoring because they are a dead code in my
build but may not be in another.
> > technique, well and good. If the data shows that this warning
> > actually is useful in finding bugs, then manybe we can figure out a
> > way that we can explicitly hint to the compiler that in *this* case,
> > the maintainer actually knew what they were doing.
> >
> > But if an examination of the warnings shows that
> > -Wunreachable-code-aggressive isn't actually finding any real bugs,
> > then perhaps it's not worth it.
>
> I agree. Hence the examination of instances found by Vinicius.
I liked the idea to create htree_rep_invariant_check. I will be doing
that.
Thanks for the help and suggestions.
> --
> Thanks,
> ~Nick Desaulniers
Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set
On Mon, Feb 1, 2021 at 2:13 PM Theodore Ts'o wrote: > > On Mon, Feb 01, 2021 at 01:15:29PM -0300, Vinicius Tinti wrote: > > On Mon, Feb 1, 2021 at 9:49 AM Christoph Hellwig wrote: > > > > > > DX_DEBUG is completely dead code, so either kill it off or make it an > > > actual CONFIG_* symbol through Kconfig if it seems useful. > > > > About the unreachable code in "if (0)" I think it could be removed. > > It seems to be doing an extra check. > > > > About the DX_DEBUG I think I can do another patch adding it to Kconfig > > as you and Nathan suggested. > > Yes, it's doing another check which is useful in terms of early > detection of bugs when a developer has the code open for > modifications. It slows down performance under normal circumstances, > and assuming the code is bug-free(tm), it's entirely unnecessary --- > which is why it's under an "if (0)". My goal is to avoid having a dead code. Three options come to mind. The first would be to add another #ifdef SOMETHING (suggest a name). But this doesn't remove the code and someone could enable it by accident. The second would be to add the code in a block comment. Then document that it is for checking an invariant. This will make it harder to cause problems. The third would be to remove the code and explain the invariant in a block comment. Maybe add a pseudo code too in the comment. What do you think? > However, if there *is* a bug, having an early detection that the > representation invariant of the data structure has been violated can > be useful in root causing a bug. This would probably be clearer if > the code was pulled out into a separate function with comments > explaining that this is a rep invariant check. Good idea. I will do that too. > The main thing about DX_DEBUG right now is that it is **super** > verbose. Unwary users who enable it will be sorry. If we want to > make it to be a first-class feature enabled via CONFIG_EXT4_DEBUG, we > should convert all of the dx_trace calls to use pr_debug so they are > enabled only if dynamic debug enables those pr_debug() statements. > And this should absolutely be a separate patch. Agreed. For now I only want to focus on the "if (0)". Regards, Vinicius > Cheers, > > - Ted
Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set
On Mon, Feb 1, 2021 at 9:49 AM Christoph Hellwig wrote: > > DX_DEBUG is completely dead code, so either kill it off or make it an > actual CONFIG_* symbol through Kconfig if it seems useful. About the unreachable code in "if (0)" I think it could be removed. It seems to be doing an extra check. About the DX_DEBUG I think I can do another patch adding it to Kconfig as you and Nathan suggested. What do you think?
[PATCH v2] ext4: Enable code path when DX_DEBUG is set
By enabling -Wunreachable-code-aggressive on Clang the following code
paths are unreachable.
This has been present since commit ac27a0ec112a ("[PATCH] ext4: initial
copy of files from ext3") and fs/ext3 had it present at the beginning of
git history. It has not been changed since.
Clang warns:
fs/ext4/namei.c:831:17: warning: code will never be executed
[-Wunreachable-code]
unsigned n = count - 1;
^
fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as
explicitly dead
if (0) { // linear search cross check
^
/* DISABLES CODE */ ( )
Signed-off-by: Vinicius Tinti
---
fs/ext4/namei.c | 23 ---
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index cf652ba3e74d..46ae6a4e4be5 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -827,20 +827,21 @@ dx_probe(struct ext4_filename *fname, struct inode *dir,
p = m + 1;
}
- if (0) { // linear search cross check
- unsigned n = count - 1;
- at = entries;
- while (n--)
+#ifdef DX_DEBUG
+ // linear search cross check
+ unsigned n = count - 1;
+ at = entries;
+ while (n--)
+ {
+ dxtrace(printk(KERN_CONT ","));
+ if (dx_get_hash(++at) > hash)
{
- dxtrace(printk(KERN_CONT ","));
- if (dx_get_hash(++at) > hash)
- {
- at--;
- break;
- }
+ at--;
+ break;
}
- ASSERT(at == p - 1);
}
+ ASSERT(at == p - 1);
+#endif
at = p - 1;
dxtrace(printk(KERN_CONT " %x->%u\n",
--
2.25.1
Re: [PATCH] drm/i915: Remove unreachable code
On Sat, Jan 30, 2021 at 9:45 AM Chris Wilson wrote:
>
> Quoting Vinicius Tinti (2021-01-30 12:34:11)
> > On Fri, Jan 29, 2021 at 08:55:54PM +, Chris Wilson wrote:
> > > Quoting Vinicius Tinti (2021-01-29 18:15:19)
> > > > By enabling -Wunreachable-code-aggressive on Clang the following code
> > > > paths are unreachable.
> > >
> > > That code exists as commentary and, especially for sdvo, library
> > > functions that we may need in future.
> >
> > I would argue that this code could be removed since it is in git history.
> > It can be restored when needed.
> >
> > This will make the code cleaner.
>
> It doesn't change the control flow, so no complexity argument. It
> removes documentation from the code, so I have the opposite opinion.
The last change in sdvo related to this function is from commit
ce22c320b8ca ("drm/i915/sdvo: convert to encoder disable/enable"), which
dates from 2012.
It has not been used or changed for a long time. I think it could be
converted to a block comment. This will preserve the documentation
purpose. What do you think?
All this will avoid having "if (0)".
> > > The ivb-gt1 case => as we now set the gt level for ivb, should we not
> > > enable the optimisation for ivb unaffected by the w/a? Just no one has
> > > taken the time to see if it causes a regression.
> >
> > I don't know. I just found out that the code is unreachable.
> >
> > > For error state, the question remains whether we should revert to
> > > uncompressed data if the compressed stream is larger than the original.
> >
> > I don't know too.
> >
> > In this last two cases the code could be commented and the decisions
> > and problems explained in the comment section.
>
> They already are, that is the point.
I meant making them a block comment. For example:
/*
* Enabling HiZ Raw Stall Optimization, at this point, causes corruption.
*
* Calling wa_masked_dis with the arguments wal, CACHE_MODE_0_GEN7,
* HIZ_RAW_STALL_OPT_DISABLE will cause an HiZ corruption on ivb:g1.
*/
/*
* Should fallback to uncompressed if we increase size
* (zstream->total_out > zstream->total_in) by returning -E2BIG?
*/
> -Chris
Re: [PATCH] drm/i915: Remove unreachable code
On Fri, Jan 29, 2021 at 08:55:54PM +, Chris Wilson wrote: > Quoting Vinicius Tinti (2021-01-29 18:15:19) > > By enabling -Wunreachable-code-aggressive on Clang the following code > > paths are unreachable. > > That code exists as commentary and, especially for sdvo, library > functions that we may need in future. I would argue that this code could be removed since it is in git history. It can be restored when needed. This will make the code cleaner. > The ivb-gt1 case => as we now set the gt level for ivb, should we not > enable the optimisation for ivb unaffected by the w/a? Just no one has > taken the time to see if it causes a regression. I don't know. I just found out that the code is unreachable. > For error state, the question remains whether we should revert to > uncompressed data if the compressed stream is larger than the original. I don't know too. In this last two cases the code could be commented and the decisions and problems explained in the comment section. > -Chris
[PATCH] ext4: Remove unreachable code
By enabling -Wunreachable-code-aggressive on Clang the following code
paths are unreachable.
Commit dd73b5d5cb67 ("ext4: convert dx_probe() to use the ERR_PTR
convention")
Commit ac27a0ec112a ("[PATCH] ext4: initial copy of files from ext3")
Clang warns:
fs/ext4/namei.c:831:17: warning: code will never be executed
[-Wunreachable-code]
unsigned n = count - 1;
^
fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as
explicitly dead
if (0) { // linear search cross check
^
/* DISABLES CODE */ ( )
Signed-off-by: Vinicius Tinti
---
fs/ext4/namei.c | 15 ---
1 file changed, 15 deletions(-)
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index cf652ba3e74d..1f64dbd7237b 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -827,21 +827,6 @@ dx_probe(struct ext4_filename *fname, struct inode *dir,
p = m + 1;
}
- if (0) { // linear search cross check
- unsigned n = count - 1;
- at = entries;
- while (n--)
- {
- dxtrace(printk(KERN_CONT ","));
- if (dx_get_hash(++at) > hash)
- {
- at--;
- break;
- }
- }
- ASSERT(at == p - 1);
- }
-
at = p - 1;
dxtrace(printk(KERN_CONT " %x->%u\n",
at == entries ? 0 : dx_get_hash(at),
--
2.25.1
[PATCH] drm/i915: Remove unreachable code
By enabling -Wunreachable-code-aggressive on Clang the following code
paths are unreachable.
Commit ce22c320b8ca ("drm/i915/sdvo: convert to encoder disable/enable")
Commit 19f1f627b333 ("drm/i915/gt: Move ivb GT workarounds from
init_clock_gating to workarounds")
Commit 0a97015d45ee ("drm/i915: Compress GPU objects in error state")
By removing the unreachable code at
drivers/gpu/drm/i915/display/intel_sdvo.c the function
intel_sdvo_set_encoder_power_state becomes unused.
Commit ea5b213ad4b1 ("drm/i915: Subclass intel_encoder.")
Clang warns unreachable:
drivers/gpu/drm/i915/display/intel_sdvo.c:1768:3: warning: code will never
be executed [-Wunreachable-code]
intel_sdvo_set_encoder_power_state(intel_sdvo,
^~
drivers/gpu/drm/i915/display/intel_sdvo.c:1767:6: note: silence by adding
parentheses to mark code as explicitly dead
if (0)
^
/* DISABLES CODE */ ( )
drivers/gpu/drm/i915/display/intel_sdvo.c:1852:3: warning: code will never
be executed [-Wunreachable-code]
intel_sdvo_set_encoder_power_state(intel_sdvo,
^~
drivers/gpu/drm/i915/display/intel_sdvo.c:1851:6: note: silence by adding
parentheses to mark code as explicitly dead
if (0)
^
/* DISABLES CODE */ ( )
drivers/gpu/drm/i915/gt/intel_workarounds.c:884:3: warning: code will never
be executed [-Wunreachable-code]
wa_masked_dis(wal, CACHE_MODE_0_GEN7,
HIZ_RAW_STALL_OPT_DISABLE);
^
drivers/gpu/drm/i915/gt/intel_workarounds.c:882:6: note: silence by adding
parentheses to mark code as explicitly dead
if (0) { /* causes HiZ corruption on ivb:gt1 */
^
/* DISABLES CODE */ ( )
drivers/gpu/drm/i915/i915_gpu_error.c:319:11: warning: code will never be
executed [-Wunreachable-code]
if (0 && zstream->total_out > zstream->total_in)
^~~
drivers/gpu/drm/i915/i915_gpu_error.c:319:6: note: silence by adding
parentheses to mark code as explicitly dead
if (0 && zstream->total_out > zstream->total_in)
^
/* DISABLES CODE */ ( )
Clang warns unused after removing unreachable:
drivers/gpu/drm/i915/display/intel_sdvo.c:696:13: warning: unused function
'intel_sdvo_set_encoder_power_state' [-Wunused-function]
static bool intel_sdvo_set_encoder_power_state(struct intel_sdvo *intel_sdvo,
^
Signed-off-by: Vinicius Tinti
---
drivers/gpu/drm/i915/display/intel_sdvo.c | 30 -
drivers/gpu/drm/i915/gt/intel_workarounds.c | 5
drivers/gpu/drm/i915/i915_gpu_error.c | 4 ---
3 files changed, 39 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
b/drivers/gpu/drm/i915/display/intel_sdvo.c
index 4eaa4aa86ecd..45d03b09f8f0 100644
--- a/drivers/gpu/drm/i915/display/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
@@ -693,30 +693,6 @@ static bool intel_sdvo_get_active_outputs(struct
intel_sdvo *intel_sdvo,
outputs, sizeof(*outputs));
}
-static bool intel_sdvo_set_encoder_power_state(struct intel_sdvo *intel_sdvo,
- int mode)
-{
- u8 state = SDVO_ENCODER_STATE_ON;
-
- switch (mode) {
- case DRM_MODE_DPMS_ON:
- state = SDVO_ENCODER_STATE_ON;
- break;
- case DRM_MODE_DPMS_STANDBY:
- state = SDVO_ENCODER_STATE_STANDBY;
- break;
- case DRM_MODE_DPMS_SUSPEND:
- state = SDVO_ENCODER_STATE_SUSPEND;
- break;
- case DRM_MODE_DPMS_OFF:
- state = SDVO_ENCODER_STATE_OFF;
- break;
- }
-
- return intel_sdvo_set_value(intel_sdvo,
- SDVO_CMD_SET_ENCODER_POWER_STATE, ,
sizeof(state));
-}
-
static bool intel_sdvo_get_input_pixel_clock_range(struct intel_sdvo
*intel_sdvo,
int *clock_min,
int *clock_max)
@@ -1764,9 +1740,6 @@ static void intel_disable_sdvo(struct intel_atomic_state
*state,
intel_sdvo_disable_audio(intel_sdvo);
intel_sdvo_set_active_outputs(intel_sdvo, 0);
- if (0)
- intel_sdvo_set_encoder_power_state(intel_sdvo,
- DRM_MODE_DPMS_OFF);
temp = intel_de_read(dev_priv, intel_sdvo->sdvo_reg);
@@ -1848,9 +1821,6 @@ static void intel_enable_sdvo(struct intel_atomic_state
*state,
"sync\n", SDVO_NAME(intel_sdvo));
}
- if (0)
- intel_sdvo_set_encoder_power_state(intel_sdvo,
- DRM_MODE_DPMS_ON);
intel_sdvo_set_active
Re: [PATCH] x86: Avoid undefined behavior in macro expansion
On Fri, Mar 18, 2016 at 11:15 PM, Al Viro <v...@zeniv.linux.org.uk> wrote: > On Wed, Mar 16, 2016 at 11:48:49PM -0300, Vinicius Tinti wrote: >> C11 standard (at 6.10.3.3) says that ## operator (paste) has undefined >> behavior when one of the result operands is not a valid preprocessing >> token. >> >> Therefore the macro expansion may depend on compiler implementation >> which may or no preserve the leading white space. >> >> Moreover other places in kernel use CONCAT(a,b) instead of CONCAT(a, b). >> Changing favors concise usage. > > Huh? > >> -#define XMM(i) CONCAT(%xmm, i) >> +#define XMM(i) CONCAT(%xmm,i) > > What are you talking about? Undefined behaviour is when the result of > concatenation of adjacent tokens is not a valid preprocessor token. > It says nothing about the either argument being a single token. Please check the example below otherwise it will be hard to explain. The problem is that _i_ can be a macro to be expanded too. And it can be a parameter for a _paste_ operator. // tricky code #define CONCAT(a,b)a##b #defineXMM(i)CONCAT(%xmm, i) .macro foo n x = XMM(\n) .endm _%xmm_ is not a problem but _i_ is. > In this case after the substitution of e.g. XMM(42) we get 3 tokens: > Punctuator[%] Identifier[xmm] Pp-number[42] > with ## instructing us to replace the last two with preprocessor token that > would be represented as concatenation of their representations. Which is > to say, concatenation of xmm and 42, i.e. xmm42. Which *is* a > representation of a valid preprocessor token - namely, Identifier[xmm42]. Agree. But it is not this case. I will add the code above at commit and describe it. It will be easy to explain what I am trying to solve. > No undefined behaviour at all. And yes, you get two preprocessor tokens > in the expansion - % and xmm42. Preprocessor works in terms of tokens, > not strings... Understood. > If you know of any compiler where these two variants would produce different > expansions of XMM(), please report it to maintainers of > the compiler in question; it's a bug, plain and simple. And no, there's > no undefined behaviour in that. I reported a bug and discussed over it and I too believe that the tricky code that I have just sent triggers an undefined behavior. What do you think? -- Simplicity is the ultimate sophistication
Re: [PATCH] x86: Avoid undefined behavior in macro expansion
On Fri, Mar 18, 2016 at 11:15 PM, Al Viro wrote: > On Wed, Mar 16, 2016 at 11:48:49PM -0300, Vinicius Tinti wrote: >> C11 standard (at 6.10.3.3) says that ## operator (paste) has undefined >> behavior when one of the result operands is not a valid preprocessing >> token. >> >> Therefore the macro expansion may depend on compiler implementation >> which may or no preserve the leading white space. >> >> Moreover other places in kernel use CONCAT(a,b) instead of CONCAT(a, b). >> Changing favors concise usage. > > Huh? > >> -#define XMM(i) CONCAT(%xmm, i) >> +#define XMM(i) CONCAT(%xmm,i) > > What are you talking about? Undefined behaviour is when the result of > concatenation of adjacent tokens is not a valid preprocessor token. > It says nothing about the either argument being a single token. Please check the example below otherwise it will be hard to explain. The problem is that _i_ can be a macro to be expanded too. And it can be a parameter for a _paste_ operator. // tricky code #define CONCAT(a,b)a##b #defineXMM(i)CONCAT(%xmm, i) .macro foo n x = XMM(\n) .endm _%xmm_ is not a problem but _i_ is. > In this case after the substitution of e.g. XMM(42) we get 3 tokens: > Punctuator[%] Identifier[xmm] Pp-number[42] > with ## instructing us to replace the last two with preprocessor token that > would be represented as concatenation of their representations. Which is > to say, concatenation of xmm and 42, i.e. xmm42. Which *is* a > representation of a valid preprocessor token - namely, Identifier[xmm42]. Agree. But it is not this case. I will add the code above at commit and describe it. It will be easy to explain what I am trying to solve. > No undefined behaviour at all. And yes, you get two preprocessor tokens > in the expansion - % and xmm42. Preprocessor works in terms of tokens, > not strings... Understood. > If you know of any compiler where these two variants would produce different > expansions of XMM(), please report it to maintainers of > the compiler in question; it's a bug, plain and simple. And no, there's > no undefined behaviour in that. I reported a bug and discussed over it and I too believe that the tricky code that I have just sent triggers an undefined behavior. What do you think? -- Simplicity is the ultimate sophistication
[PATCH] x86: Avoid undefined behavior in macro expansion
C11 standard (at 6.10.3.3) says that ## operator (paste) has undefined behavior when one of the result operands is not a valid preprocessing token. Therefore the macro expansion may depend on compiler implementation which may or no preserve the leading white space. Moreover other places in kernel use CONCAT(a,b) instead of CONCAT(a, b). Changing favors concise usage. Signed-off-by: Vinicius Tinti <viniciusti...@gmail.com> Acked-by: Behan Webster <beh...@converseincode.com> --- arch/x86/crypto/aes_ctrby8_avx-x86_64.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/crypto/aes_ctrby8_avx-x86_64.S b/arch/x86/crypto/aes_ctrby8_avx-x86_64.S index a916c4a..7a71553 100644 --- a/arch/x86/crypto/aes_ctrby8_avx-x86_64.S +++ b/arch/x86/crypto/aes_ctrby8_avx-x86_64.S @@ -93,7 +93,7 @@ #define tmp%r10 #defineDDQ(i) CONCAT(ddq_add_,i) -#defineXMM(i) CONCAT(%xmm, i) +#defineXMM(i) CONCAT(%xmm,i) #defineDDQ_DATA0 #defineXDATA 1 #define KEY_1281 -- 2.7.3
[PATCH] x86: Avoid undefined behavior in macro expansion
C11 standard (at 6.10.3.3) says that ## operator (paste) has undefined behavior when one of the result operands is not a valid preprocessing token. Therefore the macro expansion may depend on compiler implementation which may or no preserve the leading white space. Moreover other places in kernel use CONCAT(a,b) instead of CONCAT(a, b). Changing favors concise usage. Signed-off-by: Vinicius Tinti Acked-by: Behan Webster --- arch/x86/crypto/aes_ctrby8_avx-x86_64.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/crypto/aes_ctrby8_avx-x86_64.S b/arch/x86/crypto/aes_ctrby8_avx-x86_64.S index a916c4a..7a71553 100644 --- a/arch/x86/crypto/aes_ctrby8_avx-x86_64.S +++ b/arch/x86/crypto/aes_ctrby8_avx-x86_64.S @@ -93,7 +93,7 @@ #define tmp%r10 #defineDDQ(i) CONCAT(ddq_add_,i) -#defineXMM(i) CONCAT(%xmm, i) +#defineXMM(i) CONCAT(%xmm,i) #defineDDQ_DATA0 #defineXDATA 1 #define KEY_1281 -- 2.7.3
Re: [PATCH] arm64: annotate psci invoke functions as notrace
On Tue, Feb 24, 2015 at 3:11 PM, Mark Rutland wrote:
> On Tue, Feb 24, 2015 at 05:59:50PM +, Richard W.M. Jones wrote:
>> On Wed, Feb 18, 2015 at 12:26:38PM -0500, Kyle McMartin wrote:
>> > Using GCC 5 to build the kernel with ftrace enabled, we encounter the
>> > following error as a result of the mcount prologue changing the expected
>> > register use of the function parameters,
>> >
>> > /tmp/cc8Kpn7A.s: Assembler messages:
>> > /tmp/cc8Kpn7A.s:41: Error: .err encountered
>> > /tmp/cc8Kpn7A.s:42: Error: .err encountered
>> > /tmp/cc8Kpn7A.s:43: Error: .err encountered
>> > /tmp/cc8Kpn7A.s:101: Error: .err encountered
>> > /tmp/cc8Kpn7A.s:102: Error: .err encountered
>> > /tmp/cc8Kpn7A.s:103: Error: .err encountered
>> > scripts/Makefile.build:257: recipe for target 'arch/arm64/kernel/psci.o'
>> > failed
>> >
>> > Fix this by annotating the function as notrace, to suppress the
>> > generation of profiling prologues and epilogues on the function.
>> >
>> > Signed-off-by: Kyle McMartin
>> >
>> > --- a/arch/arm64/kernel/psci.c
>> > +++ b/arch/arm64/kernel/psci.c
>> > @@ -113,7 +113,7 @@ static void psci_power_state_unpack(u32 power_state,
>> > * The following two functions are invoked via the invoke_psci_fn pointer
>> > * and will not be inlined, allowing us to piggyback on the AAPCS.
>> > */
>> > -static noinline int __invoke_psci_fn_hvc(u64 function_id, u64 arg0, u64
>> > arg1,
>> > +static noinline notrace int __invoke_psci_fn_hvc(u64 function_id, u64
>> > arg0, u64 arg1,
>> > u64 arg2)
>> > {
>> > asm volatile(
>> > @@ -128,7 +128,7 @@ static noinline int __invoke_psci_fn_hvc(u64
>> > function_id, u64 arg0, u64 arg1,
>> > return function_id;
>> > }
>> >
>> > -static noinline int __invoke_psci_fn_smc(u64 function_id, u64 arg0, u64
>> > arg1,
>> > +static noinline notrace int __invoke_psci_fn_smc(u64 function_id, u64
>> > arg0, u64 arg1,
>> > u64 arg2)
>> > {
>> > asm volatile(
>>
>> I need this patch in order to compile the upstream kernel on aarch64
>> using gcc 5. Can it not be added temporarily while the longer term
>> fix, whatever that is, is worked out?
>
> As I mentioned in my reply, Will was waiting for -rc1 to post our
> patches (which move this out to asm for arm and arm64). He's out of the
> office today, but I expect they will be posted tomorrow (and hopefully
> queued shortly thereafter).
>
> Mark.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Hi,
I notice that the mainline kernel moved these psci calls to a separate
file but I
was wondering how can it guarantee that the function register
placement will hold?
If you build the kernel with -O0 some function register allocation changes as
opposed to -O2 or if you use another compiler such as Clang. In LLVMLinux we
solved this by using one of Andy's solution which is to use register placement:
register u32 function_id_r0 asm ("r0") = function_id;
register u32 arg0_r1 asm ("r1") = arg0;
register u32 arg1_r2 asm ("r2") = arg1;
register u32 arg2_r3 asm ("r3") = arg2;
Sorry by the late reply.
Regards,
Tinti
--
Simplicity is the ultimate sophistication
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] arm64: annotate psci invoke functions as notrace
On Tue, Feb 24, 2015 at 3:11 PM, Mark Rutland mark.rutl...@arm.com wrote:
On Tue, Feb 24, 2015 at 05:59:50PM +, Richard W.M. Jones wrote:
On Wed, Feb 18, 2015 at 12:26:38PM -0500, Kyle McMartin wrote:
Using GCC 5 to build the kernel with ftrace enabled, we encounter the
following error as a result of the mcount prologue changing the expected
register use of the function parameters,
/tmp/cc8Kpn7A.s: Assembler messages:
/tmp/cc8Kpn7A.s:41: Error: .err encountered
/tmp/cc8Kpn7A.s:42: Error: .err encountered
/tmp/cc8Kpn7A.s:43: Error: .err encountered
/tmp/cc8Kpn7A.s:101: Error: .err encountered
/tmp/cc8Kpn7A.s:102: Error: .err encountered
/tmp/cc8Kpn7A.s:103: Error: .err encountered
scripts/Makefile.build:257: recipe for target 'arch/arm64/kernel/psci.o'
failed
Fix this by annotating the function as notrace, to suppress the
generation of profiling prologues and epilogues on the function.
Signed-off-by: Kyle McMartin k...@redhat.com
--- a/arch/arm64/kernel/psci.c
+++ b/arch/arm64/kernel/psci.c
@@ -113,7 +113,7 @@ static void psci_power_state_unpack(u32 power_state,
* The following two functions are invoked via the invoke_psci_fn pointer
* and will not be inlined, allowing us to piggyback on the AAPCS.
*/
-static noinline int __invoke_psci_fn_hvc(u64 function_id, u64 arg0, u64
arg1,
+static noinline notrace int __invoke_psci_fn_hvc(u64 function_id, u64
arg0, u64 arg1,
u64 arg2)
{
asm volatile(
@@ -128,7 +128,7 @@ static noinline int __invoke_psci_fn_hvc(u64
function_id, u64 arg0, u64 arg1,
return function_id;
}
-static noinline int __invoke_psci_fn_smc(u64 function_id, u64 arg0, u64
arg1,
+static noinline notrace int __invoke_psci_fn_smc(u64 function_id, u64
arg0, u64 arg1,
u64 arg2)
{
asm volatile(
I need this patch in order to compile the upstream kernel on aarch64
using gcc 5. Can it not be added temporarily while the longer term
fix, whatever that is, is worked out?
As I mentioned in my reply, Will was waiting for -rc1 to post our
patches (which move this out to asm for arm and arm64). He's out of the
office today, but I expect they will be posted tomorrow (and hopefully
queued shortly thereafter).
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Hi,
I notice that the mainline kernel moved these psci calls to a separate
file but I
was wondering how can it guarantee that the function register
placement will hold?
If you build the kernel with -O0 some function register allocation changes as
opposed to -O2 or if you use another compiler such as Clang. In LLVMLinux we
solved this by using one of Andy's solution which is to use register placement:
register u32 function_id_r0 asm (r0) = function_id;
register u32 arg0_r1 asm (r1) = arg0;
register u32 arg1_r2 asm (r2) = arg1;
register u32 arg2_r3 asm (r3) = arg2;
Sorry by the late reply.
Regards,
Tinti
--
Simplicity is the ultimate sophistication
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] arm: remove extra semicolon in if statement
Remove extra semicolon in perf_event.c if statement.
Signed-off-by: Vinicius Tinti
---
arch/arm/kernel/perf_event.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 31e0eb3..a892067 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -400,7 +400,7 @@ __hw_perf_event_init(struct perf_event *event)
}
if (event->group_leader != event) {
- if (validate_group(event) != 0);
+ if (validate_group(event) != 0)
return -EINVAL;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] arm: remove extra semicolon in if statement
Remove extra semicolon in perf_event.c if statement.
Signed-off-by: Vinicius Tinti viniciusti...@gmail.com
---
arch/arm/kernel/perf_event.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 31e0eb3..a892067 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -400,7 +400,7 @@ __hw_perf_event_init(struct perf_event *event)
}
if (event-group_leader != event) {
- if (validate_group(event) != 0);
+ if (validate_group(event) != 0)
return -EINVAL;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
