Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1580062 > unrolled thread

Re: [PATCH 2/2] arch/x86: Fix sparse warning symbol not declared

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] arch/x86: Fix sparse warning symbol not declared Tobin Harding <me@tobin.cc> - 2017-02-13 21:30 +0100
    Re: [PATCH 2/2] arch/x86: Fix sparse warning symbol not declared Thomas Gleixner <tglx@linutronix.de> - 2017-02-13 22:30 +0100
      Re: [PATCH 2/2] arch/x86: Fix sparse warning symbol not declared Tobin Harding <me@tobin.cc> - 2017-02-21 09:20 +0100

#1580062 — Re: [PATCH 2/2] arch/x86: Fix sparse warning symbol not declared

On Sun, Feb 12, 2017 at 12:06:47PM +0100, Thomas Gleixner wrote:
> On Sun, 12 Feb 2017, Tobin C. Harding wrote:
> 
> > This patch adds function declaration in order to quiet sparse symbol
> > not declared warning.
> 
> Same comment vs. 'This patch' as before. Hint, we already know that this is
> a patch, otherwise it would be mislabeled.
> 
> > 
> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > ---
> > 
> > Unsure why adding declaration quiets sparse.
> 
> Because sparse finds a declaration before the definition.
> 
> > This may not be the correct solution.
> 
> Right, it's not.
> 
> > Only testing done is building and booting kernel.  Since 'purgatory' is
> > called from assembler and does not need forward declaration the only
> > advantage to this patch seems to be to save the next newbie from
> > investigating the sparse warning.
> 
> Well, yes. But just quietening a checker by slapping a pointless forward
> declaration into the code is not pretty either. A smarter checker might
> catch that.
> 
> The proper solution is to have a local include file 'purgatory.h' and put
> the declaration there. Include it in both files even if that's not required
> for the ASM file. But that documents, that the function is used outside of
> purgatory.c

Blindly following instructions led to the bone headed patch I
submitted yesterday (without building). Is there some way to include a
C header in an ASM file that I do not know about?

Thanks for patiently pointing out how to write a commit log. May I
please bother you with another small etiquette question. Should I
have replayed to you as I have done so or should I have re-sent another
patch (v3) with the mistakes fixed (and stated in the log that I did
not know how to implement the suggestions).

thanks,
Tobin.

[toc] | [next] | [standalone]

#1580086

On Tue, 14 Feb 2017, Tobin Harding wrote:
> On Sun, Feb 12, 2017 at 12:06:47PM +0100, Thomas Gleixner wrote:
> > The proper solution is to have a local include file 'purgatory.h' and put
> > the declaration there. Include it in both files even if that's not required
> > for the ASM file. But that documents, that the function is used outside of
> > purgatory.c
> 
> Blindly following instructions led to the bone headed patch I
> submitted yesterday (without building). Is there some way to include a
> C header in an ASM file that I do not know about?

Yes, you have to guard the function declaration with

#ifndef __ASSEMBLY__

I did not think about that when I suggested this. Brainslip :)

So yes, it's kinda pointless, but it still has documentatory value and
keeps the sparse build clean.

> Thanks for patiently pointing out how to write a commit log. May I
> please bother you with another small etiquette question. Should I
> have replayed to you as I have done so or should I have re-sent another
> patch (v3) with the mistakes fixed (and stated in the log that I did
> not know how to implement the suggestions).

All good. Either way works as long as you notice your own mistakes. It's
also fine to tell me that I suggested nonsense. :)

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1585104

On Tue, Feb 14, 2017, at 08:24 AM, Thomas Gleixner wrote:
> On Tue, 14 Feb 2017, Tobin Harding wrote:
> > On Sun, Feb 12, 2017 at 12:06:47PM +0100, Thomas Gleixner wrote:
> > > The proper solution is to have a local include file 'purgatory.h' and put
> > > the declaration there. Include it in both files even if that's not required
> > > for the ASM file. But that documents, that the function is used outside of
> > > purgatory.c
> > 
> > Blindly following instructions led to the bone headed patch I
> > submitted yesterday (without building). Is there some way to include a
> > C header in an ASM file that I do not know about?
> 
> Yes, you have to guard the function declaration with
> 
> #ifndef __ASSEMBLY__
> 
> I did not think about that when I suggested this. Brainslip :)
> 
> So yes, it's kinda pointless, but it still has documentatory value and
> keeps the sparse build clean.
> 
> > Thanks for patiently pointing out how to write a commit log. May I
> > please bother you with another small etiquette question. Should I
> > have replayed to you as I have done so or should I have re-sent another
> > patch (v3) with the mistakes fixed (and stated in the log that I did
> > not know how to implement the suggestions).
> 
> All good. Either way works as long as you notice your own mistakes. It's
> also fine to tell me that I suggested nonsense. :)
> 

It has been pointed out to me that I could have approached the dev
process of this patch better. In the name of learning the correct method
I am now replying to your comments Thomas. Thanks for being patient, if
you could please slap me if I slip I should be able to blossom into a
decent contributor.

After sending an incorrect version (v3) with the __ASSEMBLY__
preprocessor guard in the wrong place, I re-sent v4 with what I believe
is all of your comments implemented and functioning.

Thanks for taking the time to review my annoying multi-version almost
trivial patch series.

thanks,
Tobin.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web