Re: [PATCH] Documentation/sparse.txt: document context annotations for lock checking
On Thu, Oct 18, 2012 at 7:27 AM, Ed Cashin wrote: > The context feature of sparse is used with the Linux kernel > sources to check for imbalanced uses of locks. Document the > annotations defined in include/linux/compiler.h that tell sparse > what to expect when a lock is held on function entry, exit, or > both. > > Signed-off-by: Ed Cashin Signed-off-by: Christopher Li Chris -- 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] Re: sparse breakage triggered by rcu_read_lock() lockdep annotations
OK, I get a trivial fix after all. The test case is fixed now.
I haven't done much test otherwise.
See the patch attached.
Chris
On 10/19/07, Chris Li <[EMAIL PROTECTED]> wrote:
> Err,
>
> Sparse does not support the local label syntax yet. It just treats the
> second label "x:" as the same as the first one. Then the linearize
> code gets serious confused when it saw one label get define in two
> places.
>
> The fix seems not trivial from the first look.
>
> Chris
>
> On 10/16/07, Alexey Dobriyan <[EMAIL PROTECTED]> wrote:
> > FWIW, commit 851a67b825540a8e00c0be3ee25e4627ba8b133b
> > aka "lockdep: annotate rcu_read_{,un}lock{,_bh}"
> > causes sparse to trigger internal assertion in quite a few places over
> > allyesconfig run.
> >
> > sparse: flow.c:805: rewrite_parent_branch: Assertion `changed'
> > failed.
> >
> > Trimmed down testcase:
> >
> > void f(unsigned long ip);
> > static void g(void)
> > {
> > if (1) {
> > f(({ __label__ x; x: (unsigned long)&&x; }));
> > }
> > f(({ __label__ x; x: (unsigned long)&&x; }));
> > }
> >
> > #0 0x4001c410 in __kernel_vsyscall ()
> > (gdb) bt
> > #0 0x4001c410 in __kernel_vsyscall ()
> > #1 0x40050701 in raise () from /lib/libc.so.6
> > #2 0x40051e38 in abort () from /lib/libc.so.6
> > #3 0x40049fcc in __assert_fail () from /lib/libc.so.6
> > #4 0x08064947 in pack_basic_blocks (ep=0x411a1c6c) at flow.c:812
> > #5 0x0805ffbf in linearize_symbol (sym=0x4103ec8c) at linearize.c:2154
> > #6 0x080492a3 in main (argc=Cannot access memory at address 0x274d) at
> > sparse.c:266
> >
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
> > the body of a message to [EMAIL PROTECTED]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
local-label
Description: Binary data
compat ioctl for submiting URB
gt;iso_frame_desc[0],
- sizeof(struct usbdevfs_iso_packet_desc) *
- kurb->number_of_packets))
- return -EFAULT;
-
- totlen = 0;
- for (i = 0; i < kurb->number_of_packets; i++) {
- unsigned int this_len;
-
- this_len = kurb->iso_frame_desc[i].length;
- if (this_len > 1023)
- return -EINVAL;
-
- totlen += this_len;
- }
-
- if (totlen > 32768)
- return -EINVAL;
- kurb->buffer_length = totlen;
-
- return 0;
-}
-
-static int do_usbdevfs_urb(unsigned int fd, unsigned int cmd, unsigned long
arg)
-{
- struct usbdevfs_urb *kurb;
- struct usbdevfs_urb32 *uurb;
- mm_segment_t old_fs;
- __u32 udata;
- void *uptr, *kptr;
- unsigned int buflen;
- int err;
-
- uurb = compat_ptr(arg);
-
- err = -ENOMEM;
- kurb = kmalloc(sizeof(struct usbdevfs_urb) +
- (sizeof(struct usbdevfs_iso_packet_desc) * 128),
- GFP_KERNEL);
- if (!kurb)
- goto out;
-
- err = -EFAULT;
- if (get_urb32(kurb, uurb))
- goto out;
-
- err = get_urb32_isoframes(kurb, uurb);
- if (err)
- goto out;
-
- err = -EFAULT;
- if (__get_user(udata, &uurb->buffer))
- goto out;
- uptr = compat_ptr(udata);
-
- buflen = kurb->buffer_length;
- err = verify_area(VERIFY_WRITE, uptr, buflen);
- if (err)
- goto out;
-
-
- old_fs = get_fs();
- set_fs(KERNEL_DS);
- err = sys_ioctl(fd, USBDEVFS_SUBMITURB, (unsigned long) kurb);
- set_fs(old_fs);
-
- if (err >= 0) {
- /* RED-PEN Shit, this doesn't work for async URBs :-( XXX */
- if (put_urb32(kurb, uurb)) {
- err = -EFAULT;
- }
- }
-
-out:
- kfree(kurb);
- return err;
-}
-#endif
+/*
+ * The USBDEVFS_SUBMITURB, USBDEVFS_REAPURB and USBDEVFS_REAPURBNDELAY
+ * is handle in usbdevfs core. -Christopher Li
+ */
#define USBDEVFS_REAPURB32 _IOW('U', 12, u32)
#define USBDEVFS_REAPURBNDELAY32 _IOW('U', 13, u32)
static int do_usbdevfs_reapurb(unsigned int fd, unsigned int cmd, unsigned
long arg)
{
-mm_segment_t old_fs;
-void *kptr;
-int err;
-
-old_fs = get_fs();
-set_fs(KERNEL_DS);
-err = sys_ioctl(fd,
-(cmd == USBDEVFS_REAPURB32 ?
- USBDEVFS_REAPURB :
- USBDEVFS_REAPURBNDELAY),
-(unsigned long) &kptr);
-set_fs(old_fs);
-
-if (err >= 0 &&
-put_user((u32)(u64)kptr, (u32 __user *)compat_ptr(arg)))
-err = -EFAULT;
-
-return err;
+ cmd = (cmd == USBDEVFS_REAPURB32) ? USBDEVFS_REAPURB :
USBDEVFS_REAPURBNDELAY;
+ return sys_ioctl(fd, cmd, arg);
}
struct usbdevfs_disconnectsignal32 {
@@ -3332,7 +3123,6 @@
/* Usbdevfs */
HANDLE_IOCTL(USBDEVFS_CONTROL32, do_usbdevfs_control)
HANDLE_IOCTL(USBDEVFS_BULK32, do_usbdevfs_bulk)
-/*HANDLE_IOCTL(USBDEVFS_SUBMITURB32, do_usbdevfs_urb)*/
HANDLE_IOCTL(USBDEVFS_REAPURB32, do_usbdevfs_reapurb)
HANDLE_IOCTL(USBDEVFS_REAPURBNDELAY32, do_usbdevfs_reapurb)
HANDLE_IOCTL(USBDEVFS_DISCSIGNAL32, do_usbdevfs_discsignal)
Index: linux-2.5/drivers/usb/core/urb.c
===
--- linux-2.5.orig/drivers/usb/core/urb.c 2005-01-25 12:55:19.0
-0800
+++ linux-2.5/drivers/usb/core/urb.c2005-01-28 16:35:14.0 -0800
@@ -312,6 +312,7 @@
allowed = URB_ASYNC_UNLINK; // affects later unlinks
allowed |= (URB_NO_TRANSFER_DMA_MAP | URB_NO_SETUP_DMA_MAP);
allowed |= URB_NO_INTERRUPT;
+ allowed |= URB_COMPAT;
switch (temp) {
case PIPE_BULK:
if (is_out)
Index: linux-2.5/drivers/usb/core/devio.c
===
--- linux-2.5.orig/drivers/usb/core/devio.c 2005-01-25 12:08:25.0
-0800
+++ linux-2.5/drivers/usb/core/devio.c 2005-01-28 16:35:14.0 -0800
@@ -803,9 +803,11 @@
return status;
}
-static int proc_submiturb(struct dev_state *ps, void __user *arg)
+
+static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
+struct usbdevfs_iso_packet_desc __user
*iso_frame_desc,
+void __user *arg)
{
- struct usbdevfs_urb uurb;
struct usbdevfs_iso_packet_desc *isopkt = NULL;
struct usb_host_endpoint *ep;
struct async *as;
@@ -813,42 +815,40 @@
unsigned int u, totlen, isofrmlen;
int ret, interval = 0, ifnum = -1;
- if (copy_from_user(&uurb, ar
Re: compat ioctl for submiting URB
This patch is for the case that running 32 bit application on a 64 bit kernel. So far only x86_64 allow you to do that. I am not aware of other 64bit architecture need the 32bit emulation. Chris On Sat, Jan 29, 2005 at 04:29:51AM +, Gianni Tedesco wrote: > On Fri, 2005-01-28 at 16:23 -0500, Christopher Li wrote: > > +#ifdef CONFIG_IA32_EMULATION > > + > > + case USBDEVFS_SUBMITURB32: > > + snoop(&dev->dev, "%s: SUBMITURB32\n", __FUNCTION__); > > + ret = proc_submiturb_compat(ps, p); > > + if (ret >= 0) > > + inode->i_mtime = CURRENT_TIME; > > + break; > > +#endif > > Why don't other 64bit architectures need this chunk? > > -- > // Gianni Tedesco (gianni at scaramanga dot co dot uk) > lynx --source www.scaramanga.co.uk/scaramanga.asc | gpg --import > - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: compat ioctl for submiting URB
It is nice to know all that. I guess I did not know much about the other 64 bit systems. I will update and resend my patch. Thanks! Chris On Fri, Jan 28, 2005 at 09:45:38PM -0800, Roland Dreier wrote: > Christopher> This patch is for the case that running 32 bit > Christopher> application on a 64 bit kernel. So far only x86_64 > Christopher> allow you to do that. > > Actually, at least ia64, mips, parisc, ppc64, s390 and sparc64 also > support 32-bit applications on a 64-bit kernel. All of those > architectures except s390 can use USB. I guess vmware doesn't run on > most of those architectures but any solution in the mainline kernel > should be generic enough to handle them all. > > - R. > - > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to [EMAIL PROTECTED] > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: compat ioctl for submiting URB
On Sat, Jan 29, 2005 at 07:33:31AM +0100, Andi Kleen wrote: > > Looks reasonable from a first look. > > Issues: > - Should use CONFIG_COMPAT, not x86-64 specific symbols Agree. > - Why can't you set URB_COMPAT transparently in the emulation > layer? Then existing applications would hopefully work without > changes, right? The existing application is don't need to set the USB_COMPAT flag. It is use internally to track the URB is submit from 32 bit user space. I guess I don't have to use that flag. > > You may also want to preserve the __user casts, otherwise > Al Viro and other sparse users will be unhappy. I did try. Which place are you referring to? I guess miss some of it. Chris > > Thanks for attacking this long standing problem. > > -Andi > > - > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to [EMAIL PROTECTED] > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] compat USB ioctl take II was Re: compat ioctl for submiting URB
return 0;
-}
-
-static int do_usbdevfs_urb(unsigned int fd, unsigned int cmd, unsigned long
arg)
-{
- struct usbdevfs_urb *kurb;
- struct usbdevfs_urb32 *uurb;
- mm_segment_t old_fs;
- __u32 udata;
- void *uptr, *kptr;
- unsigned int buflen;
- int err;
-
- uurb = compat_ptr(arg);
-
- err = -ENOMEM;
- kurb = kmalloc(sizeof(struct usbdevfs_urb) +
- (sizeof(struct usbdevfs_iso_packet_desc) * 128),
- GFP_KERNEL);
- if (!kurb)
- goto out;
-
- err = -EFAULT;
- if (get_urb32(kurb, uurb))
- goto out;
-
- err = get_urb32_isoframes(kurb, uurb);
- if (err)
- goto out;
-
- err = -EFAULT;
- if (__get_user(udata, &uurb->buffer))
- goto out;
- uptr = compat_ptr(udata);
-
- buflen = kurb->buffer_length;
- err = verify_area(VERIFY_WRITE, uptr, buflen);
- if (err)
- goto out;
-
- old_fs = get_fs();
- set_fs(KERNEL_DS);
- err = sys_ioctl(fd, USBDEVFS_SUBMITURB, (unsigned long) kurb);
- set_fs(old_fs);
-
- if (err >= 0) {
- /* RED-PEN Shit, this doesn't work for async URBs :-( XXX */
- if (put_urb32(kurb, uurb)) {
- err = -EFAULT;
- }
- }
-
-out:
- kfree(kurb);
- return err;
-}
-#endif
-
-#define USBDEVFS_REAPURB32 _IOW('U', 12, u32)
-#define USBDEVFS_REAPURBNDELAY32 _IOW('U', 13, u32)
-
-static int do_usbdevfs_reapurb(unsigned int fd, unsigned int cmd, unsigned
long arg)
-{
-mm_segment_t old_fs;
-void *kptr;
-int err;
-
-old_fs = get_fs();
-set_fs(KERNEL_DS);
-err = sys_ioctl(fd,
- (cmd == USBDEVFS_REAPURB32 ?
- USBDEVFS_REAPURB :
- USBDEVFS_REAPURBNDELAY),
-(unsigned long) &kptr);
-set_fs(old_fs);
-
-if (err >= 0 &&
-put_user((u32)(u64)kptr, (u32 __user *)compat_ptr(arg)))
-err = -EFAULT;
-
-return err;
-}
+/*
+ * The USBDEVFS_SUBMITURB, USBDEVFS_REAPURB and USBDEVFS_REAPURBNDELAY
+ * is handle in usbdevfs core. -Christopher Li
+ */
struct usbdevfs_disconnectsignal32 {
compat_int_t signr;
@@ -3332,9 +3114,6 @@
/* Usbdevfs */
HANDLE_IOCTL(USBDEVFS_CONTROL32, do_usbdevfs_control)
HANDLE_IOCTL(USBDEVFS_BULK32, do_usbdevfs_bulk)
-/*HANDLE_IOCTL(USBDEVFS_SUBMITURB32, do_usbdevfs_urb)*/
-HANDLE_IOCTL(USBDEVFS_REAPURB32, do_usbdevfs_reapurb)
-HANDLE_IOCTL(USBDEVFS_REAPURBNDELAY32, do_usbdevfs_reapurb)
HANDLE_IOCTL(USBDEVFS_DISCSIGNAL32, do_usbdevfs_discsignal)
/* i2c */
HANDLE_IOCTL(I2C_FUNCS, w_long)
Index: linux-2.5/drivers/usb/core/devio.c
===dø©
--- linux-2.5.orig/drivers/usb/core/devio.c 2005-01-25 12:08:25.0
-0800
+++ linux-2.5/drivers/usb/core/devio.c 2005-01-29 01:23:08.0 -0800
@@ -803,9 +803,11 @@
return status;
}
-static int proc_submiturb(struct dev_state *ps, void __user *arg)
+
+static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
+struct usbdevfs_iso_packet_desc __user
*iso_frame_desc,
+void __user *arg)
{
- struct usbdevfs_urb uurb;
struct usbdevfs_iso_packet_desc *isopkt = NULL;
struct usb_host_endpoint *ep;
struct async *as;
@@ -813,42 +815,40 @@
unsigned int u, totlen, isofrmlen;
int ret, interval = 0, ifnum = -1;
- if (copy_from_user(&uurb, arg, sizeof(uurb)))
- return -EFAULT;
- if (uurb.flags & ~(USBDEVFS_URB_ISO_ASAP|USBDEVFS_URB_SHORT_NOT_OK|
+ if (uurb->flags & ~(USBDEVFS_URB_ISO_ASAP|USBDEVFS_URB_SHORT_NOT_OK|
URB_NO_FSBR|URB_ZERO_PACKET))
return -EINVAL;
- if (!uurb.buffer)
+ if (!uurb->buffer)
return -EINVAL;
- if (uurb.signr != 0 && (uurb.signr < SIGRTMIN || uurb.signr > SIGRTMAX))
+ if (uurb->signr != 0 && (uurb->signr < SIGRTMIN || uurb->signr >
SIGRTMAX))
return -EINVAL;
- if (!(uurb.type == USBDEVFS_URB_TYPE_CONTROL && (uurb.endpoint &
~USB_ENDPOINT_DIR_MASK) == 0)) {
- if ((ifnum = findintfep(ps->dev, uurb.endpoint)) < 0)
+ if (!(uurb->type == USBDEVFS_URB_TYPE_CONTROL && (uurb->endpoint &
~USB_ENDPOINT_DIR_MASK) == 0)) {
+ if ((ifnum = findintfep(ps->dev, uurb->endpoint)) < 0)
return ifnum;
if ((ret = checkintf(ps, ifnum)))
return ret;
}
- if ((uurb.endpoint & ~USB_
Re: Adding a field to ext2_dir_entry_2
That design sounds bad. Anyway, I guess the error you are getting might have some thing to do with the "." and ".." entry. Your current directory need to in the same format as well. That is the price you pay for breaking the compatibility. Chris On Mon, Apr 04, 2005 at 08:08:57PM -0400, Vineet Joglekar wrote: > > Hi Andreas, > > I have created another file system - copied everything from ext2, renaming it > as some different file system and doing some experiments on that. > > Let me be more clear about what I am trying to do. In my masters project, I > am encrypting inodes along with the data part of the file. Keys of different > users are different. In the same directory, if there are 2 files stored by > different users, their inodes will be encrypted with different keys. If user1 > is doing "ls" on that directory, the inode of the other file - which is > encrypted by user2, will be decrypted by using user1's key, resulting into > garbage. To avoid this, I am trying to store the uid in the directry entry, > so that I can match it with current->fsuid and skip decrypting the inode if > the file doesn't belong to the current user. (assuming user1 doesnt want to > share that file and different users can store different files under same > directory.) > > Thanks and regards, > > Vineet > > --- On Mon 04/04, Andreas Dilger < [EMAIL PROTECTED] > wrote: > From: Andreas Dilger [mailto: [EMAIL PROTECTED] > To: [EMAIL PROTECTED] > Cc: linux-kernel@vger.kernel.org, linux-c-programming@vger.kernel.org > Date: Mon, 4 Apr 2005 17:17:59 -0600 > Subject: Re: Adding a field to ext2_dir_entry_2 > > On Apr 04, 2005 18:54 -0400, Vineet Joglekar wrote:> I working with > linux kernel 2.4.28. I want to add 1 more field to> ext2_dir_entry_2 - > the new version of directory entry for ext2fs.> > I did add the __u32 > field to the struct ext2_dir_entry_2 defined in> ext2_fs.h I also > modified the EXT2_DIR_REC_LEN macro to:> > (((name_len) + 12 + > EXT2_DIR_ROUND) & ~EXT2_DIR_ROUND)> > (+12 instead of +8) to > incorporate newly added 4 bytes field.> > I made the similar changes > to the mke2fs utility also.This means your filesystem will not be > mountable by any other version ofLinux. What is more important is why > you want to do this - there areother mechanisms that may be more > appropriate depending on what you aredoing.Cheers, > Andreas--Andreas DilgerPrincipal Software EngineerCluster > File Systems, Inc.Attachment: Attachment (0.19KB) > > ___ > Join Excite! - http://www.excite.com > The most personalized portal on the Web! > - > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to [EMAIL PROTECTED] > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: more git updates..
On Sun, Apr 10, 2005 at 12:51:59AM -0700, Junio C Hamano wrote: > > But I am wondering what your plans are to handle renames---or > does git already represent them? > Rename should just work. It will create a new tree object and you will notice that in the entry that changed, the hash for the blob object is the same. Chris - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: more git updates..
On Sat, Apr 09, 2005 at 04:31:10PM -0700, Linus Torvalds wrote:
>
> Done, and pushed out. The current git.git repository seems to do all of
> this correctly.
>
> NOTE! This means that each "tree" file basically tracks just a single
> directory. The old style of "every file in one tree file" still works, but
> fsck-cache will warn about it. Happily, the git archive itself doesn't
> have any subdirectories, so git itself is not impacted by it.
That is really cool stuff. My way to read it, correct me if I am wrong,
git is a user space version file system. "tree" <--> directory and
"blob" <--> file. "commit" to describe the version history.
Git always write out a full new version of blob when there is any
update to it. At first I think that waste a lot of space, especially
when there is only tiny change to it. But the more I think about it,
it make more sense. Kernel source is usually small objects and file is
compressed store any way. A very useful thing to gain form it is that,
we can truncate the older history. e.g. We can have option not to sync
the pre 2.4 change set, only grab it if we need to. Most of the time we
only interested in the recent change set.
There is one problem though. How about the SHA1 hash collision?
Even the chance is very remote, you don't want to lose some data do due
to "software" error. I think it is OK that no handle that
case right now. On the other hand, it will be nice to detect that
and give out a big error message if it really happens.
Some thing like the following patch, may be turn off able.
Chris
Index: git-0.03/read-cache.c
===
--- git-0.03.orig/read-cache.c 2005-04-09 18:42:16.0 -0400
+++ git-0.03/read-cache.c 2005-04-10 02:48:36.0 -0400
@@ -210,8 +210,22 @@
int fd;
fd = open(filename, O_WRONLY | O_CREAT | O_EXCL, 0666);
- if (fd < 0)
- return (errno == EEXIST) ? 0 : -1;
+ if (fd < 0) {
+ void *map;
+ static int error(const char * string);
+
+ if (errno != EEXIST)
+ return -1;
+ fd = open(filename, O_RDONLY);
+ if (fd < 0)
+ return -1;
+ map = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
+ if (map == MAP_FAILED)
+ return -1;
+ if (memcmp(buf, map, size))
+ return error("Ouch, Strike by lighting!\n");
+ return 0;
+ }
write(fd, buf, size);
close(fd);
return 0;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: more git updates..
On Sun, Apr 10, 2005 at 02:28:54AM -0700, Junio C Hamano wrote: > >>>>> "CL" == Christopher Li <[EMAIL PROTECTED]> writes: > > CL> On Sun, Apr 10, 2005 at 12:51:59AM -0700, Junio C Hamano wrote: > >> > >> But I am wondering what your plans are to handle renames---or > >> does git already represent them? > >> > > CL> Rename should just work. It will create a new tree object and you > CL> will notice that in the entry that changed, the hash for the blob > CL> object is the same. > > Sorry, I was unclear. But doesn't that imply that a SCM built > on top of git storage needs to read all the commit and tree > records up to the common ancestor to show tree diffs between two > forked tree? > > I suspect that another problem is that noticing the move of the > same SHA1 hash from one pathname to another and recognizing that > as a rename would not always work in the real world, because > sometimes people move files *and* make small changes at the same > time. If git is meant to be an intermediate format to suck > existing kernel history out of BK so that the history can be > converted for the next SCM chosen for the kernel work, I would > imagine that there needs to be a way to represent such a case. > Maybe convert a file rename as two git trees (one tree for pure > move which immediately followed by another tree for edit) if it > is not a pure move? > Git is not a SCM yet. For the rename + change set it should internally handle by pure rename only plus the extra delta. The current git don't have per file change history. From git's point of view some file deleted and the other file appeared with same content. It is the top level SCM to handle that correctly. Rename a directory will be even more fun. Chris - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Re: more git updates..
On Sun, Apr 10, 2005 at 11:41:53AM +0200, Petr Baudis wrote: > Dear diary, on Sun, Apr 10, 2005 at 07:53:40AM CEST, I got a letter > where Christopher Li <[EMAIL PROTECTED]> told me that... > > On Sun, Apr 10, 2005 at 12:51:59AM -0700, Junio C Hamano wrote: > > > > > > But I am wondering what your plans are to handle renames---or > > > does git already represent them? > > > > > > > Rename should just work. It will create a new tree object and you > > will notice that in the entry that changed, the hash for the blob > > object is the same. > > Which is of course wrong when you want to do proper merging, examine > per-file history, etc. One solution which springs to my mind is to have > a UUID accompany each blob and tree; that will take relatively lot of > space though, and I'm not sure it is really worth it. It should just use the rename + change two step then it is tractable with git now. Chris - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: more git updates..
I totally agree that odds is really really small. That is why it is not worthy to handle the case. People hit that can just add a new line or some thing to avoid it, if it happen after all. It is the little peace of mind to know for sure that did not happen. I am just paranoid. Chris On Sun, Apr 10, 2005 at 12:23:52PM -0700, Paul Jackson wrote: > > Some thing like the following patch, may be turn off able. > > Take out an old envelope and compute on it the odds of this > happening. > > Say we have 10,000 kernel hackers, each producing one > new file every minute, for 100 hours a week. And we've > cloned a small army of Andrew Morton's to integrate > the resulting tsunamai of patches. And Linus is well > cared for in the state funny farm. > > What is the probability that this check will fire even > once, between now and 10 billion years from now, when > the Sun has become a red giant destroying all life on > planet Earth? > > -- > I won't rest till it's the best ... > Programmer, Linux Scalability > Paul Jackson <[EMAIL PROTECTED]> 1.650.933.1373, > 1.925.600.0401 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: more git updates..
On Sun, Apr 10, 2005 at 01:57:33PM -0700, Linus Torvalds wrote: > > > That way of thinking really doesn't work well here. > > > > I will have to look more closely at pasky's GIT toolkit > > if I want to see an SCM style interface. > > Yes. You really should think of GIT as a filesystem, and of me as a > _systems_ person, not an SCM person. In fact, I tend to detest SCM's. I > think the reason I worked so well with BitKeeper is that Larry used to do > operating systems. He's also a systems person, not really an SCM person. > Or at least he's in between the two. > Yes, I am puzzled for a while how to use git until I realize that it is a version file system. BTW, one thing I learn from ext3 is that it is very useful to have some compatible flag for future development. I think if we want to reserve some room in the file format for further development of git, it is the right time to do it before it get bigs. e.g. an optional variable size header in "tree" including format version and capability etc. I can see the counter argument that it is not as important as a real file system because it is a lot easier bring it off line to upgrade the whole tree. One the other hand, it is almost did not cost any thing in terms of space and CPU time, most directory did not get to file system block boundary so extra few bytes is almost free. If carefully planed, it will make the future up grade of git a lot smoother. What do you think? Chris - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: more git updates..
On Sun, Apr 10, 2005 at 03:38:39PM -0700, Linus Torvalds wrote: > > > On Sun, 10 Apr 2005, Christopher Li wrote: > > > > BTW, one thing I learn from ext3 is that it is very useful to have some > > compatible flag for future development. I think if we want to reserve some > > room in the file format for further development of git > > Way ahead of you. > > This is (one reason) why all git objects have the type embedded inside of > them. The format of all objects is totally regular: they are all > compressed with zlib, they are all named by the sha1 file, and they all > start out with a magic header of " ". > > So if I want to create a new kind of tree object that does the same thing > as the old one but has some other layout, I'd just call it something else. > Like "dir". That was what I initially planned to do about the change to > recursive tree objects, but it turned out to actually be a lot easier to > just encode it in the old type (that way the routines that read it don't > even have to care about old/new types - it's all the same to them). Ha, that is right. You put the new type into same object trick me into thinking I have to do the same way. Totally forget I can introduce new type of objects. It is even cleaner. Cool. How about deleting trees from the caches? I don't need to delete stuff from the official tree. It is more for my local version control. Here is the usage case, - I check out the git.git. - using quilt to build my series of patches, git-hack1, git-hack2.. git-hack6. let's say those are store in git cache as well - I pick some of them come up with a clean one "submit.patch" - submit.patch get merged into official git tree. - Now I want to get rid of the hack1 to hack6, but how? One way to do it is never commit hack1 to hack6 into git or cache. They stay as quilt patches only. But it is very tempting to let quilt using git instead of the .pc/ directory, quilt can simplify as some usage case of patch and git. Chris - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: more git updates..
I see. It just need some basic set operation (+, -, and)
and some way to select a set:
sha5--->
/
/
sha1-->sha2-->sha3--
\/
\ /
>sha4
list sha1 # all the file list in changeset sha1
# {sha1}
list sha1,sha1 # same as above
list sha1,sha2 # all the file list in between changeset sha1
# and changeset sha2
# {sha1, sha2} in example
list sha1,sha3 # {sha1, sha2, sha3, sha4}
list sha1,any # all the change set reachable from sha1.
{sha1, ... sha5, ...}
new sha1,sha2 # all the new file add between in sha1, sha2 (+)
changed sha1,sha2 # add the changed file between sha1, sha2 (>) (<)
deleted sha1,sha2 # add the deleted file between sha1, sha2(-)
before time # all the file before time
aftertime # all the file after time
So in my example, the file I want to delete is :
{list hack1, base}+ {list hack2, base} ... {list hack6, base} \
- [list official_merge, base ]
On Sun, Apr 10, 2005 at 04:21:08PM -0700, Linus Torvalds wrote:
>
>
> > the official tree. It is more for my local version control.
>
> I have a plan. Namely to have a "list-needed" command, which you give one
> commit, and a flag implying how much "history" you want (*), and then it
> spits out all the sha1 files it needs for that history.
>
> Then you delete all the other ones from your SHA1 archive (easy enough to
> do efficiently by just sorting the two lists: the list of "needed" files
> and the list of "available" files).
>
> Script that, and call the command "prune-tree" or something like that, and
> you're all done.
>
> (*) The amount of history you want might be "none", which is to say that
> you don't want to go back in time, so you want _just_ the list of tree and
> blob objects associated with that commit.
That will be {list head}
>
> Or you might want a "linear" history, which would be the longest path
> through the parent changesets to the root.
That will be {list head,root}
>
> Or you might want "all", which would follow all parents and all trees.
That will be {list any, root}
>
> Or you might want to prune the history tree by date - "give me all
> history, but cut it off when you hit a parent that was done more than 6
> months ago".
That is {after -6month }
>
> This "list-needed" thing is not just for pruning history either. If you
> have a local tree "x", and you want to figure out how much of it you need
> to send to somebody else who has an older tree "y", then what you'd do is
> basically "list-needed x" and remove the set of "list-needed y". That
> gives you the answer to the question "what's the minimum set of sha1 files
> I need to send to the other guy so that he can re-create my top-of-tree".
>
That is {list x, any} - {list y, any}
> My second plan is to make somebody else so fired up about the problem that
> I can just sit back and take patches. That's really what I'm best at.
> Sitting here, in the (rain) on the patio, drinking a foofy tropical drink,
> and pressing the "apply" button. Then I take all the credit for my
> incredible work.
Sounds like a good plan.
Chris
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] resend: compat ioctl for submiting URB
__u32 udata;
- void *uptr, *kptr;
- unsigned int buflen;
- int err;
-
- uurb = compat_ptr(arg);
-
- err = -ENOMEM;
- kurb = kmalloc(sizeof(struct usbdevfs_urb) +
- (sizeof(struct usbdevfs_iso_packet_desc) * 128),
- GFP_KERNEL);
- if (!kurb)
- goto out;
-
- err = -EFAULT;
- if (get_urb32(kurb, uurb))
- goto out;
-
- err = get_urb32_isoframes(kurb, uurb);
- if (err)
- goto out;
-
- err = -EFAULT;
- if (__get_user(udata, &uurb->buffer))
- goto out;
- uptr = compat_ptr(udata);
-
- buflen = kurb->buffer_length;
- err = verify_area(VERIFY_WRITE, uptr, buflen);
- if (err)
- goto out;
-
- old_fs = get_fs();
- set_fs(KERNEL_DS);
- err = sys_ioctl(fd, USBDEVFS_SUBMITURB, (unsigned long) kurb);
- set_fs(old_fs);
-
- if (err >= 0) {
- /* RED-PEN Shit, this doesn't work for async URBs :-( XXX */
- if (put_urb32(kurb, uurb)) {
- err = -EFAULT;
- }
- }
-
-out:
- kfree(kurb);
- return err;
-}
-#endif
-
-#define USBDEVFS_REAPURB32 _IOW('U', 12, u32)
-#define USBDEVFS_REAPURBNDELAY32 _IOW('U', 13, u32)
-
-static int do_usbdevfs_reapurb(unsigned int fd, unsigned int cmd, unsigned
long arg)
-{
-mm_segment_t old_fs;
-void *kptr;
-int err;
-
-old_fs = get_fs();
-set_fs(KERNEL_DS);
-err = sys_ioctl(fd,
- (cmd == USBDEVFS_REAPURB32 ?
- USBDEVFS_REAPURB :
- USBDEVFS_REAPURBNDELAY),
-(unsigned long) &kptr);
-set_fs(old_fs);
-
-if (err >= 0 &&
-put_user((u32)(u64)kptr, (u32 __user *)compat_ptr(arg)))
-err = -EFAULT;
-
-return err;
-}
+/*
+ * USBDEVFS_SUBMITURB, USBDEVFS_REAPURB and USBDEVFS_REAPURBNDELAY
+ * are handled in usbdevfs core. -Christopher Li
+ */
struct usbdevfs_disconnectsignal32 {
compat_int_t signr;
@@ -3332,9 +3114,6 @@
/* Usbdevfs */
HANDLE_IOCTL(USBDEVFS_CONTROL32, do_usbdevfs_control)
HANDLE_IOCTL(USBDEVFS_BULK32, do_usbdevfs_bulk)
-/*HANDLE_IOCTL(USBDEVFS_SUBMITURB32, do_usbdevfs_urb)*/
-HANDLE_IOCTL(USBDEVFS_REAPURB32, do_usbdevfs_reapurb)
-HANDLE_IOCTL(USBDEVFS_REAPURBNDELAY32, do_usbdevfs_reapurb)
HANDLE_IOCTL(USBDEVFS_DISCSIGNAL32, do_usbdevfs_discsignal)
/* i2c */
HANDLE_IOCTL(I2C_FUNCS, w_long)
Index: linux-2.5/drivers/usb/core/devio.c
===
--- linux-2.5.orig/drivers/usb/core/devio.c 2005-02-07 15:10:54.0
-0800
+++ linux-2.5/drivers/usb/core/devio.c 2005-02-07 15:10:54.0 -0800
@@ -816,9 +816,11 @@
return status;
}
-static int proc_submiturb(struct dev_state *ps, void __user *arg)
+
+static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
+struct usbdevfs_iso_packet_desc __user
*iso_frame_desc,
+void __user *arg)
{
- struct usbdevfs_urb uurb;
struct usbdevfs_iso_packet_desc *isopkt = NULL;
struct usb_host_endpoint *ep;
struct async *as;
@@ -826,42 +828,40 @@
unsigned int u, totlen, isofrmlen;
int ret, interval = 0, ifnum = -1;
- if (copy_from_user(&uurb, arg, sizeof(uurb)))
- return -EFAULT;
- if (uurb.flags & ~(USBDEVFS_URB_ISO_ASAP|USBDEVFS_URB_SHORT_NOT_OK|
+ if (uurb->flags & ~(USBDEVFS_URB_ISO_ASAP|USBDEVFS_URB_SHORT_NOT_OK|
URB_NO_FSBR|URB_ZERO_PACKET))
return -EINVAL;
- if (!uurb.buffer)
+ if (!uurb->buffer)
return -EINVAL;
- if (uurb.signr != 0 && (uurb.signr < SIGRTMIN || uurb.signr > SIGRTMAX))
+ if (uurb->signr != 0 && (uurb->signr < SIGRTMIN || uurb->signr >
SIGRTMAX))
return -EINVAL;
- if (!(uurb.type == USBDEVFS_URB_TYPE_CONTROL && (uurb.endpoint &
~USB_ENDPOINT_DIR_MASK) == 0)) {
- if ((ifnum = findintfep(ps->dev, uurb.endpoint)) < 0)
+ if (!(uurb->type == USBDEVFS_URB_TYPE_CONTROL && (uurb->endpoint &
~USB_ENDPOINT_DIR_MASK) == 0)) {
+ if ((ifnum = findintfep(ps->dev, uurb->endpoint)) < 0)
return ifnum;
if ((ret = checkintf(ps, ifnum)))
return ret;
}
- if ((uurb.endpoint & ~USB_ENDPOINT_DIR_MASK) != 0)
- ep = ps->dev->ep_in [uurb.endpoint & USB_ENDPOINT_NUMBER_MASK];
+ if ((uurb->endpoint & ~USB_ENDPOINT_DIR_MASK) != 0)
+ ep = ps->d
[patch] bug fix in usbdevfs
Hi,
I am sorry that the last patch about 32 bit compat ioctl on
64 bit kernel actually breaks the usbdevfs. That is on the current
BK tree. I am retarded.
Here is the patch to fix it. Tested with USB hard disk and webcam
in both 32bit compatible mode and native 64bit mode.
Again, sorry about that.
Chris
Index: linux-2.5/drivers/usb/core/devio.c
===
--- linux-2.5.orig/drivers/usb/core/devio.c 2005-03-30 18:19:50.0
-0800
+++ linux-2.5/drivers/usb/core/devio.c 2005-03-30 19:35:31.0 -0800
@@ -1032,15 +1032,15 @@
if (put_user(urb->error_count, &userurb->error_count))
return -EFAULT;
- if (!(usb_pipeisoc(urb->pipe)))
- return 0;
- for (i = 0; i < urb->number_of_packets; i++) {
- if (put_user(urb->iso_frame_desc[i].actual_length,
-&userurb->iso_frame_desc[i].actual_length))
- return -EFAULT;
- if (put_user(urb->iso_frame_desc[i].status,
-&userurb->iso_frame_desc[i].status))
- return -EFAULT;
+ if (usb_pipeisoc(urb->pipe)) {
+ for (i = 0; i < urb->number_of_packets; i++) {
+ if (put_user(urb->iso_frame_desc[i].actual_length,
+&userurb->iso_frame_desc[i].actual_length))
+ return -EFAULT;
+ if (put_user(urb->iso_frame_desc[i].status,
+&userurb->iso_frame_desc[i].status))
+ return -EFAULT;
+ }
}
free_async(as);
@@ -1126,7 +1126,7 @@
if (get_urb32(&uurb,(struct usbdevfs_urb32 *)arg))
return -EFAULT;
- return proc_do_submiturb(ps, &uurb, ((struct usbdevfs_urb __user
*)arg)->iso_frame_desc, arg);
+ return proc_do_submiturb(ps, &uurb, ((struct usbdevfs_urb32 __user
*)arg)->iso_frame_desc, arg);
}
static int processcompl_compat(struct async *as, void __user * __user *arg)
@@ -1146,15 +1146,15 @@
if (put_user(urb->error_count, &userurb->error_count))
return -EFAULT;
- if (!(usb_pipeisoc(urb->pipe)))
- return 0;
- for (i = 0; i < urb->number_of_packets; i++) {
- if (put_user(urb->iso_frame_desc[i].actual_length,
-&userurb->iso_frame_desc[i].actual_length))
- return -EFAULT;
- if (put_user(urb->iso_frame_desc[i].status,
-&userurb->iso_frame_desc[i].status))
- return -EFAULT;
+ if (usb_pipeisoc(urb->pipe)) {
+ for (i = 0; i < urb->number_of_packets; i++) {
+ if (put_user(urb->iso_frame_desc[i].actual_length,
+&userurb->iso_frame_desc[i].actual_length))
+ return -EFAULT;
+ if (put_user(urb->iso_frame_desc[i].status,
+&userurb->iso_frame_desc[i].status))
+ return -EFAULT;
+ }
}
free_async(as);
@@ -1177,10 +1177,8 @@
{
struct async *as;
- printk("reapurbnblock\n");
if (!(as = async_getcompleted(ps)))
return -EAGAIN;
- printk("reap got as %p\n", as);
return processcompl_compat(as, (void __user * __user *)arg);
}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[ANNOUNCE] sparse-0.2-cl2 is now available
Temporarily at: http://userweb.kernel.org/~chrisl/sparse-0.2-cl2 Will appear later at: http://ftp.kernel.org//pub/linux/kernel/people/chrisl/patches/sparse/sparse-0.2-cl2/ I have been play with sparse to add more Stanford checker style of checking. The paper is "Checking System Rules Using System- Specific, Programmer-Written Compiler Extensions" by Dawson Engler etc. Unlike the Stanford checker and smatch, this checker is working on the linearization level instead of AST level. Linearization code can be very convenient (when it works) to trace the data flow because pseudo is in SSA form. There is define/user chain to avoid scan every instruction. I take the malloc checking for example to explain how the checker works. The checking usually happen in three step: The first step is scanning the linearize instruction. It look for relevant operations. For malloc checker, the task is find out the malloc/free function call and usage of malloced pointer. The second step is converting the relevant operations into checker instruction. The checker instruction is a simplification of the whole program, only contain the operation relevant to checker. The third step is executing the checker instruction. It try to execute every possible execution flow in the function. The execution engine will let the checker instruction perform state changes. Thanks to step two, the size and complexity of the of program has been greatly reduced. The new checking has been very fast, it add a few seconds to the make C=1 run. Again, comment and feed back are always welcome. Chris Change log in sparse-0.2-cl2: - adding pointer signedness fix - adding spinlock checking Change log in sparse-0.2-cl1: The most interesting part is the inline function annotation. The new checker can find out inlined function usage. The interrupt checker does not depend on x86 asm instruction any more. origin.patch 006eff06c7adcfb0d06c6fadf6e9b64f0488b2bf URL: git://git.kernel.org/pub/scm/linux/kernel/git/josh/sparse.git incompatible-ptr-signess Bug fix in pointer modifiers inherent at function degeneration. sizeof-incomplete Fix double semicolon in struct declare anon-symbol Fix core dump on anonymous symbol. instruction-buffer-size Fix core dump on huge switch debug-checker Adding debug option for showing the linearized instruction. no-dead-instruction Disable liveness "dead" instruction by default. ptr-allocator Make the ptrlist using the sparse allocator. annotate-inline-2 Add annotation for inline function call. malloc-checker Adding the malloc NULL pointer checker. interrupt-checker Adding the interrupt checker spinlock-checker Adding spinlock checker Total 12 patches - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ANNOUNCE] sparse-0.2-cl2 is now available
On Sat, Feb 10, 2007 at 06:33:25PM +0100, Andi Kleen wrote: > Interesting. Did you find any kernel bugs with this? In short, not very useful yet. The current run of of sparse-0.2-cl2 on git default i386 config will find about 6 place kernel using allocated memory without NULL check. But Linus said most of them is not worthy checking because it is in the early stage of the kernel initializations. It just can't fail on those small memory allocation. May be one of them worth adding the NULL check. For interrupt and spinlock checking, it less noisier than the current sparse context level checking. The new checker code can identify inline function call, so it has more information. But it is still too noisy. I did not look at every interrupt and spinlock warning. From what I saw, it show limit of the checker itself rather than a bug in kernel. A lot of false positive is come from we don't have enough information inside a single function. e.g. sparse has not way to know some function only get called with interrupt disabled (or some lock already hold). So it assume interrupt is still enable and generate wrong warnings. Another example is that some helper function will wrap the locking function. Complain about the exit with locking hold is wrong. I am hoping adding the cross function checking will reduce those false positive. Any way, it need more information to reduce false positive. I am still working on the cross function checking. May be it will become more useful one day. Chris - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ANNOUNCE] sparse-0.2-cl2 is now available
On Sun, Feb 11, 2007 at 05:50:15AM +, Al Viro wrote: > > I have some stuff in that direction, but it take some resurrecting... OK, we should talk. Here is what I have: Linearize bytecode writer, which produce the binary linearized code. The uncompress size is about 10 times the i386 .o file. I don't have the loader ready to verify it yet. It is aim at fast loading bytecode and simple, I havn't done much toward optimized the code size. If we keep it 10 times over head, my home computer can load the full linux kernel and have some spare for checking. I am still working on the bytecode loader and linker for merging symbols. It need to answer the question: Which file define which function. Which external symbol does this function use. Once we get the function user/define chain, it can enable a lot of new checking. The current linearized code is not very friendly to linking because we keep abstract declare and real declare as different node. Depend on the position, the caller will get different node even in one file. I am going to take out the LKML on later email because discussion is more sparse specific now. Chris - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add const to pointer qualifiers for __chk_user_ptr and __chk_io_ptr.
On Mon, Mar 26, 2007 at 11:23:56AM -0400, Russ Cox wrote: > Change prototypes for __chk_user_ptr and __chk_io_ptr > to take const void* instead of void*, so that code can pass > const void* to them. (Right now sparse does not warn > about passing const void* to void* functions, but that > is a separate bug that I believe Josh is working on, > and once sparse does check this, the changed prototypes > will be necessary.) I don't think it is needed. The __user has noderef attribute. Which means it is not allow to dereference the pointer. The const qualifier allow read dereference, only write is not allowed. Adding const here will likely force the caller to do a cast at the pointer arguments. Which defeats the checker. Sparse allow passing const void* to void* is a different issue. Chris - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add const to pointer qualifiers for __chk_user_ptr and __chk_io_ptr.
On Mon, Mar 26, 2007 at 02:59:39PM -0400, Russ Cox wrote: > No, you have it backward. > It is valid to pass void* to a const void* function. > It is *not* valid to pass const void* to a void* function. > > Right now __chk_user_ptr is a void* function, meaning > that all the places where it gets passed a const void* > are technically illegal -- gcc would warn about these, and > it is a (separate, as you observed) bug that sparse does not. > > The patch changes __chk_user_ptr to be a const void* > function, meaning that it will be legal to pass either void* > or const void* to it. This is the correct semantics. Hah, I see. Thanks for the explain. Ack. Chris - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [next:akpm 798/1000] drivers/rtc/rtc-ds1286.c:343:24: sparse: incorrect type in argument 1 (different address spaces)
On Mon, Apr 22, 2013 at 4:56 PM, Andrew Morton
wrote:
> I think doing IS_ERR() and PTR_ERR() on __iomem pointers is a natural
> thing, and we should be able to do this without adding call-site
> trickery to make sparse happy.
>
> Is there some sort of annotation which we can add to the
> IS_ERR()/PTR_ERR() definitions so that sparse will stop warning about
> this usage?
Yes, the force attribute should silent the address check on conversion.
Can some one try this patch (totally untested).
Chris
diff --git a/include/linux/err.h b/include/linux/err.h
index f2edce2..d226a3c 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -26,17 +26,17 @@ static inline void * __must_check ERR_PTR(long error)
static inline long __must_check PTR_ERR(const void *ptr)
{
- return (long) ptr;
+ return (__force long) ptr;
}
static inline long __must_check IS_ERR(const void *ptr)
{
- return IS_ERR_VALUE((unsigned long)ptr);
+ return IS_ERR_VALUE((__force unsigned long)ptr);
}
static inline long __must_check IS_ERR_OR_NULL(const void *ptr)
{
- return !ptr || IS_ERR_VALUE((unsigned long)ptr);
+ return !ptr || IS_ERR_VALUE((__force unsigned long)ptr);
}
/**
--
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: [trace:tip/perf/core 5/15] kernel/trace/trace_functions.c:79:16: sparse: incorrect type in initializer (different address spaces)
On Sat, Mar 2, 2013 at 5:23 AM, Fengguang Wu wrote: > That's it! Verified with > > make C=1 kernel/trace/trace_functions.o You can use C=2 to run sparse without recompile the obj file. Chris -- 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] err.h: IS_ERR() can accept __user pointers
On Wed, May 8, 2013 at 12:42 AM, Dan Carpenter wrote: > Sparse generates a false positive when you pass a __user or __iomem > pointer to the IS_ERR() functions. > > drivers/rtc/rtc-ds1286.c:344:36: sparse: incorrect type in argument 1 > (different address spaces) > drivers/rtc/rtc-ds1286.c:344:36:expected void const *ptr > drivers/rtc/rtc-ds1286.c:344:36:got unsigned int [noderef] [usertype] > *rtcregs > > We can silence these by adding a __force here and upgrading to the > latest git release of Sparse. > > Signed-off-by: Dan Carpenter > --- > This change has no effect when using current Sparse releases. Acked. Chris -- 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 V2] sparse: Allow override of sizeof(bool) warning
On Wed, Feb 26, 2014 at 8:32 PM, H. Peter Anvin wrote: > > Quite frankly, this is silly in my opinion, *and* it is not guaranteed > by C either (read about "trap representations"). >> > Anything that moves data around in a generic fashion. It can be as > simple as: > > memcpy(foo, bar, sizeof *foo); OK. I get it nobody wants a sizeof(_Bool) warning. I am going to apply this patch. Should we change the default to off then? Thanks. Chris -- 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 V2] sparse: Allow override of sizeof(bool) warning
On Thu, Feb 27, 2014 at 12:26 PM, H. Peter Anvin wrote:
> I would.
>
Joe, I assume you are OK with this patch, the default is now off.
Chris
Allow an override to emit or not the sizeof(bool) warning.
Add a "-Wsizeof-bool" description to the manpage.
Signed-off-by: Joe Perches
Reviewed-by: Josh Triplett
Signed-off-by: Christopher Li
---
evaluate.c | 3 ++-
lib.c | 2 ++
lib.h | 1 +
sparse.1 | 8
4 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/evaluate.c b/evaluate.c
index 6655615..a45f59b 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2057,7 +2057,8 @@ static struct symbol *evaluate_sizeof(struct
expression *expr)
}
if (size == 1 && is_bool_type(type)) {
-warning(expr->pos, "expression using sizeof bool");
+if (Wsizeof_bool)
+warning(expr->pos, "expression using sizeof bool");
size = bits_in_char;
}
diff --git a/lib.c b/lib.c
index 51b97fd..844797d 100644
--- a/lib.c
+++ b/lib.c
@@ -226,6 +226,7 @@ int Wparen_string = 0;
int Wptr_subtraction_blows = 0;
int Wreturn_void = 0;
int Wshadow = 0;
+int Wsizeof_bool = 0;
int Wtransparent_union = 0;
int Wtypesign = 0;
int Wundef = 0;
@@ -438,6 +439,7 @@ static const struct warning {
{ "ptr-subtraction-blows", &Wptr_subtraction_blows },
{ "return-void", &Wreturn_void },
{ "shadow", &Wshadow },
+{ "sizeof-bool", &Wsizeof_bool },
{ "transparent-union", &Wtransparent_union },
{ "typesign", &Wtypesign },
{ "undef", &Wundef },
diff --git a/lib.h b/lib.h
index f09b338..f6cd9b4 100644
--- a/lib.h
+++ b/lib.h
@@ -120,6 +120,7 @@ extern int Wparen_string;
extern int Wptr_subtraction_blows;
extern int Wreturn_void;
extern int Wshadow;
+extern int Wsizeof_bool;
extern int Wtransparent_union;
extern int Wtypesign;
extern int Wundef;
diff --git a/sparse.1 b/sparse.1
index cd6be26..54da09b 100644
--- a/sparse.1
+++ b/sparse.1
@@ -297,6 +297,14 @@ Such declarations can lead to error-prone code.
Sparse does not issue these warnings by default.
.
.TP
+.B \-Wsizeof-bool
+Warn when checking the sizeof a _Bool.
+
+C99 does not specify the sizeof a _Bool. gcc uses 1.
+
+Sparse does not issue these warnings by default.
+.
+.TP
.B \-Wtransparent\-union
Warn about any declaration using the GCC extension
\fB__attribute__((transparent_union))\fR.
--
1.8.5.3
--
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 V2] sparse: Allow override of sizeof(bool) warning
On Thu, Feb 27, 2014 at 12:39 PM, Joe Perches wrote:
> Please use V3 as I stuffed up the alphabetic order
> of sizeof and shadow.
Please send it your V3 then :-)
>
> I'm not sure it matters much, but the linux-kernel
> Makefile wouldn't need to be changed if Wsizeof_bool
> is default 0.
It seems default to off is the right thing to do.
>
> Here's a couple of other nits:
>
> Maybe the evaluate.c "size = bits_in_char;" assignment
>
> if (size == 1 && is_bool_type(type)) {
> - warning(expr->pos, "expression using sizeof bool");
> + if (Wsizeof_bool)
> + warning(expr->pos, "expression using sizeof bool");
> size = bits_in_char;
> }
>
> should be
>
> size = sizeof(_Bool) * 8;
The reason to use bits_in_ is to allow sparse application to over write
the size of int etc. If you don't like the bits_in_char here. You can introduce
bits_in_bool and set that to sizeof(Bool)*8 by default. That way you don't
hard code it.
> And also, in sparse.1, Josh Triplett is shown as
> the maintainer. Maybe that should be changed to
> Christopher Li
Maybe a separate patch.
Waiting for your V3.
Chris
--
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 V2] sparse: Allow override of sizeof(bool) warning
On Thu, Feb 27, 2014 at 1:00 PM, Joe Perches wrote: > On Thu, 2014-02-27 at 12:44 -0800, Christopher Li wrote: >> On Thu, Feb 27, 2014 at 12:26 PM, H. Peter Anvin wrote: >> > I would. >> Joe, I assume you are OK with this patch, the default is now off. > > Of course Let me know if you are going to have a V3 or not. Thanks Chris -- 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 V2] sparse: Allow override of sizeof(bool) warning
On Thu, Feb 27, 2014 at 1:00 PM, Joe Perches wrote: > Of course > The change has applied and pushed. Chris -- 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] staging: rtl8192u: initialize array in C compliant way
On Tue, May 6, 2014 at 1:47 AM, Dan Carpenter wrote:
>> diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_TSProc.c
>> b/drivers/staging/rtl8192u/ieee80211/rtl819x_TSProc.c
>> index 426f223..c96dbab 100644
>> --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_TSProc.c
>> +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_TSProc.c
>> @@ -241,7 +241,7 @@ static PTS_COMMON_INFO SearchAdmitTRStream(struct
>> ieee80211_device *ieee,
>> {
>> //DIRECTION_VALUE dir;
>> u8 dir;
>> - boolsearch_dir[4] = {0, 0, 0, 0};
>> + boolsearch_dir[4] = {0};
>
> That's weird. The original code is valid but it generates a sparse
> warning.
Sorry for the late reply. I see what is going on already. The bits_to_bytes()
function is buggy for bool type. Bool is the only type has bits less than 8.
It truncates the bool byte size to 0. As a result, the array layout function
convert_index() always set the expr->init_offset to 0 for bool type:
"e->init_offset = from * bits_to_bytes(e->ctype->bit_size);"
Chris
--
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] staging: rtl8192u: initialize array in C compliant way
On Tue, May 6, 2014 at 1:47 AM, Dan Carpenter wrote:
>> - boolsearch_dir[4] = {0, 0, 0, 0};
>> + boolsearch_dir[4] = {0};
>
> That's weird. The original code is valid but it generates a sparse
> warning.
>
> drivers/staging/rtl8192u/ieee80211/rtl819x_TSProc.c:244:58: warning:
> Initializer entry defined twice
> drivers/staging/rtl8192u/ieee80211/rtl819x_TSProc.c:244:61: also defined
> here
>
> It has something to do with "_Bool" types. Changing it to "int" will
> also make the warning disappear. I've CC'd the sparse list to see if
> anyone knows what's happening.
This is easy reproducible with a small test case. It is a sparse bug for sure.
Let me take a look.
Chris
--
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/
[RFC PATCH] kbuild: avoid some unnecessary rebuild
In the current tip of git tree. If I do a allmodconfig full build, then follow by a "make" again without changing any code in the tree. There are some object will always get rebuild. e.g. eboot.o. I use "make V=2", it shows that the rebuild is due to some object file not in $(targets). I did not understand the $(targets) very well, the Kbuild.include mention that It is likely a bug in the kbuild if that happen. I add them in the $(targets) and that seems avoid the unnecessary rebuild of several objects. The second "make" has fewer output. The clean rebuild seems works fine too. Chris kbuild-avoid-rebuild-some-object.patch Description: Binary data
Re: [RFC PATCH] kbuild: avoid some unnecessary rebuild
On Sun, Mar 17, 2013 at 2:58 PM, Sam Ravnborg wrote: > > We got no patch - just some git help stuff. > Seems you mistyped somthing.. > Oops, sorry about that. I reattach the patch here. Chris kbuild-avoid-rebuild-some-object.patch Description: Binary data
Re: [RFC PATCH] kbuild: avoid some unnecessary rebuild
Let me add a signed off in case some one want to apply it. Signed-Off-By: Christopher Li On Wed, Mar 20, 2013 at 1:19 PM, Sam Ravnborg wrote: > I looked at the attached patch: > Acked-by: Sam Ravnborg -- 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: Suggestion for fixing the variable length array used in the kernel.
On Fri, Mar 8, 2013 at 9:39 PM, Dan Carpenter wrote: > On Fri, Mar 08, 2013 at 04:29:22PM -0800, Andrew Morton wrote: >> Roughly how many instances of this are there kernel-wide? >> > > Around 150 on x86 allmodconfig. They are pretty well audited. I saw 207 on x86-64 allmodconfig. See the list that I attached. Can you elaborate the well audited part? How it was audited? I try to figure out if it is worth the trouble to fix it. Chris var-array-error Description: Binary data
Re: Suggestion for fixing the variable length array used in the kernel.
On Sat, Mar 9, 2013 at 2:34 PM, Dan Carpenter wrote:
> The problems is if we go over the 8k stack. So big arrays are bad.
> Also if the dynamically sized array is inside a loop then normally
> GCC frees it after each iteration, but on some arches it didn't free
> it until after the last iteration.
So it seems that you agree those variable array usage should be
better change to use kmalloc or some thing.
> Btw, I've Smatch has cross function analysis, and I'd like to use
> it here to figure out if the max size for dynamically sized arrays.
> I ran into a problem:
>
> The code looks like this:
> char buf[a];
> The size expression should be an EXPR_SYMBOL, but smatch gets:
> char buf[*a];
Sparse currently does not deal with the dynamic array size right now.
It only want to get constant value from the array size.
The part that evaluate the array size is actually correct. Remember
the EXPR_SYMBOL
actually contain the *address* of symbol "a". So the proper
sizeof(buf) is actually
the content of "*a". That part is fine.
The more complicated case of dynamic array size is using the dynamic array in
a struct:
struct {
char descriptor1[length+1];
char descriptor2[length+1];
} *d;
Then the sizeof(*d) need to be ((*length) + 1 + (*length) + 1), assume
"length" is a
symbol address. The sizeof (struct foo) can be pretty complicate expression.
Some USB code use this kind of the dynamic array. However, it does not allocate
the struct in the stack, the struct is allocated via kmalloc using pointer.
Sparse still complain the variable length array though.
Let me see if I can make the sparse handle dynamic array better.
Chris
--
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] forced argument Was Re: sparse: incorrect type in argument 1 (different address spaces)
On 04/22/2013 11:16 PM, Dan Carpenter wrote:
> That didn't work. It's the the void * in the parameter list that's
> the problem. We'd need to do something like the patch below:
>
> Otherwise we could add "__ok_to_cast" thing to Sparse maybe?
Thanks for the insight. I make a small patch to test the __ok_to_cast
feature. The syntax is adding the force attribute to the argument
declaration.
it will look like this:
static inline long __must_check PTR_ERR( __force const void *ptr)
That means the "ptr" argument will perform a forced cast when receiving
the argument. It is OK to pass __iomem pointer to "ptr".
The example are in the patch. It need to patch both sparse and the
Linux tree.
What do you say?
Chris
>From a0974ed0fc1e67c41608c780b748c205622956b8 Mon Sep 17 00:00:00 2001
From: Christopher Li
Date: Thu, 25 Apr 2013 18:09:43 -0700
Subject: [PATCH] Allow forced attribute in function argument
It will indicate this argument will skip the compatible check.
---
evaluate.c | 2 +-
parse.c| 1 +
symbol.h | 3 ++-
validation/fored_arg.c | 18 ++
4 files changed, 22 insertions(+), 2 deletions(-)
create mode 100644 validation/fored_arg.c
diff --git a/evaluate.c b/evaluate.c
index 9f2c4ac..0dfa519 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2137,7 +2137,7 @@ static int evaluate_arguments(struct symbol *f, struct symbol *fn, struct expres
else
degenerate(expr);
}
- } else {
+ } else if (!target->forced_arg){
static char where[30];
examine_symbol_type(target);
sprintf(where, "argument %d", i);
diff --git a/parse.c b/parse.c
index 45ffc10..890e56b 100644
--- a/parse.c
+++ b/parse.c
@@ -1841,6 +1841,7 @@ static struct token *parameter_declaration(struct token *token, struct symbol *s
sym->ctype = ctx.ctype;
sym->ctype.modifiers |= storage_modifiers(&ctx);
sym->endpos = token->pos;
+ sym->forced_arg = ctx.storage_class == SForced;
return token;
}
diff --git a/symbol.h b/symbol.h
index 1e74579..1c6ad66 100644
--- a/symbol.h
+++ b/symbol.h
@@ -157,7 +157,8 @@ struct symbol {
expanding:1,
evaluated:1,
string:1,
- designated_init:1;
+ designated_init:1,
+ forced_arg:1;
struct expression *array_size;
struct ctype ctype;
struct symbol_list *arguments;
diff --git a/validation/fored_arg.c b/validation/fored_arg.c
new file mode 100644
index 000..4ab7141
--- /dev/null
+++ b/validation/fored_arg.c
@@ -0,0 +1,18 @@
+/*
+ * check-name: Forced function argument type.
+ */
+
+#define __iomem __attribute__((noderef, address_space(2)))
+#define __force __attribute__((force))
+
+static void foo(__force void * addr)
+{
+}
+
+
+static void bar(void)
+{
+ void __iomem *a;
+ foo(a);
+}
+
--
1.8.1.4
Suggestion for fixing the variable length array used in the kernel.
Hi,
I am looking at the current sparse warning on the kernel source.
One category of those warning are produce by the variable length array.
We all know that the kernel stack has a limit so we don't want to allocate
too much stack to the variable size array.
Is there a recommended way to fix those warnings? Is it worth while to
fix it at all? I am looking forward to some kind of guideline how to handle
this.
Some of them has estimated size limited, like the one fournd in decode_rs.c
/* Err+Eras Locator poly and syndrome poly The maximum value
* of nroots is 8. So the necessary stack size will be about
* 220 bytes max.
*/
uint16_t lambda[nroots + 1], syn[nroots];
uint16_t b[nroots + 1], t[nroots + 1], omega[nroots + 1];
uint16_t root[nroots], reg[nroots + 1], loc[nroots];
Some of them did not said the size estimation but you kind of know
they are not likely to blow up the stack:
In xen_flush_tlb_others
struct {
struct mmuext_op op;
#ifdef CONFIG_SMP
DECLARE_BITMAP(mask, num_processors);
#else
DECLARE_BITMAP(mask, NR_CPUS);
#endif
} *args;
And also some of them are harder to tell from the context if
there will be a size limit:
int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)
{
unsigned int k;
struct snd_pcm_hardware *hw;
struct snd_interval *i = NULL;
struct snd_mask *m = NULL;
struct snd_pcm_hw_constraints *constrs =
&substream->runtime->hw_constraints;
unsigned int rstamps[constrs->rules_num];
<-
Chris
--
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 v2 2/2] lib.c: skip --param parameters
-- 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 v2 2/2] lib.c: skip --param parameters
Oops, I just click send before I type up the reply. Here we go again.
On Tue, Jun 17, 2014 at 2:11 AM, Andy Shevchenko
wrote:
> Very dumb patch to just skip --param allow-store-data-races=0 introduced in
> newer GCC versions.
>
> +static char **handle_param(char *arg, char **next)
> +{
> + const char *value = NULL;
> +
> + /* For now just skip any '--param=*' or '--param *' */
> + value = split_value_from_arg(arg, value);
> + if (!value)
> + ++next;
> +
> + return ++next;
> +}
I think this is problematic.There are three possible input
from args:
1) "--parm", you need to ++next skip to next arg, which is the value for parm.
2) "--parm=x", you don't need to skip to next arg.
3) "--parm-with-crap", invalid argument. You don't need to skip next arg.
I think the patch is wrong on case 2) and case 3).
In case 2), the patch skip two arguments and make next point
points to out of bound memory.
The split_value_from_arg function is not a good abstraction for this job.
Its return value can only indicate 2 possible out come.
Also, returning the default value force the test against the input
default value. That make the logic a bit complicate.
> struct switches {
> const char *name;
> char **(*fn)(char *, char **);
> @@ -686,13 +698,14 @@ struct switches {
> static char **handle_long_options(char *arg, char **next)
> {
> static struct switches cmd[] = {
> + { "param", handle_param },
> { "version", handle_version },
> { NULL, NULL }
> };
> struct switches *s = cmd;
>
> while (s->name) {
> - if (!strcmp(s->name, arg))
> + if (!strncmp(arg, s->name, strlen(s->name)))
This will allow "--version-with-crap" as valid arguments.
I think we can have one extra member in "struct switch"
to indicate this option is a prefix rather than a whole word.
For "parm", it need to set that prefix member to non zero.
Please let me know if there is a V3 coming.
Chris
--
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 v2 2/2] lib.c: skip --param parameters
Hi Andy,
On Sat, Jun 28, 2014 at 9:59 AM, Christopher Li wrote:
> I think this is problematic.There are three possible input
> from args:
> 1) "--parm", you need to ++next skip to next arg, which is the value for parm.
> 2) "--parm=x", you don't need to skip to next arg.
> 3) "--parm-with-crap", invalid argument. You don't need to skip next arg.
How about this patch, I modify from your patch.
It fix the problem I mention earlier.
If no objections, I will push the change.
Chris
From d917662d54ba68d0c3b03e994cb1fa66d7b19c30 Mon Sep 17 00:00:00 2001
From: Andy Shevchenko
Date: Tue, 17 Jun 2014 12:11:45 +0300
Subject: [PATCH] lib.c: skip --param parameters
Very dumb patch to just skip --param allow-store-data-races=0 introduced in
newer GCC versions.
Without this patch sparse recognizes parameter of the --param option as a file
name which obviously couldn't be found.
Signed-off-by: Andy Shevchenko
Signed-off-by: Christopher Li
---
lib.c | 24 ++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/lib.c b/lib.c
index 844797d..0bc4b2b 100644
--- a/lib.c
+++ b/lib.c
@@ -675,22 +675,42 @@ static char **handle_version(char *arg, char **next)
exit(0);
}
+static char **handle_param(char *arg, char **next)
+{
+ char *value = NULL;
+
+ /* For now just skip any '--param=*' or '--param *' */
+ if (*arg == '\0') {
+ value = *++next;
+ } else if (isspace(*arg) || *arg == '=') {
+ value = ++arg;
+ }
+
+ if (!value)
+ die("missing argument for --param option");
+
+ return next;
+}
+
struct switches {
const char *name;
char **(*fn)(char *, char **);
+ unsigned int prefix:1;
};
static char **handle_long_options(char *arg, char **next)
{
static struct switches cmd[] = {
+ { "param", handle_param, 1 },
{ "version", handle_version },
{ NULL, NULL }
};
struct switches *s = cmd;
while (s->name) {
- if (!strcmp(s->name, arg))
- return s->fn(arg, next);
+ int optlen = strlen(s->name);
+ if (!strncmp(s->name, arg, optlen + !s->prefix))
+ return s->fn(arg + optlen, next);
s++;
}
return next;
--
1.9.3
Re: [PATCH v2 2/2] lib.c: skip --param parameters
On Sun, Jun 29, 2014 at 12:19 AM, Christopher Li wrote: > > If no objections, I will push the change. > Change pushed. Chris -- 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 v2 2/2] lib.c: skip --param parameters
On Mon, Jun 30, 2014 at 1:32 AM, Andy Shevchenko
wrote:
>
> Hmm... I'd just added test printf to the handle_param() and see if I
> print *next, it is either --param or --param=*. So, using return (next +
> 2) helps, otherwise we end up with the same situation as before patch.
The return value from handle_switch() is a bit tricky. It is actually points to
the current args which about to be expired.
Take a look at this code which invoke the handle_switch().
for (;;) {
char *arg = *++args; < notice the ++
before the fetch
if (!arg)
break;
if (arg[0] == '-' && arg[1]) {
args = handle_switch(arg+1, args); < args return here.
continue;
}
add_ptr_list_notag(filelist, arg);
}
>
> What did I miss?
So the caller loop will perform 1 pointer advance before fetch.
Your code can advance 2 pointer, so that is total 3 pointer advance.
>
> Which was explicitly mentioned in the commit message.
Sorry about that, I jump to the code first. I later notice that in
the commit message as well.
Any way, the change I push should fix all that.
Chris
--
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] sparse: Add CLOG option for saving warning
Currently sparse warning only output to stderr.
In the parallel build process, different source
file output is fixed together. It is hard to keep
track of the warning.
Add the CLOG= option in command line to
save the sparse warning into individual log file.
Typical usage:
make -j8 C=2 CLOG=
The log file is saved in the target directory as
.xxx.o..sparse
By diffing between different log file, it is much
easier to analyze how the sparse change impact
the whole kernel build.
Signed-off-by: Christopher Li
Chris
From 3b2ff204cbda684adf9dba2adf568062533ae34d Mon Sep 17 00:00:00 2001
From: Christopher Li
Date: Mon, 30 Jun 2014 01:33:22 -0700
Subject: [PATCH] sparse: Add CLOG option for saving warning
Currently sparse warning only output to stderr.
In the parallel build process, different source
file output is fixed together. It is hard to keep
track of the warning.
Add the CLOG= option in command line to
save the sparse warning into individual log file.
Typical usage:
make -j8 C=2 CLOG=
The log file is saved in the target directory as
.xxx.o..sparse
By diffing between different log file, it is much
easier to analyze how the sparse change impact
the whole kernel build.
Signed-off-by: Christopher Li
---
Makefile | 13 -
scripts/Makefile.build | 11 +--
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index b11e2d5..56c3502 100644
--- a/Makefile
+++ b/Makefile
@@ -68,6 +68,16 @@ ifndef KBUILD_CHECKSRC
KBUILD_CHECKSRC = 0
endif
+ifeq ("$(origin CLOG)", "command line")
+ KBUILD_CHECKLOG = $(CLOG)
+endif
+ifndef KBUILD_CHECKLOG
+ KBUILD_CHECKLOG =
+endif
+
+
+
+
# Use make M=dir to specify directory of external module to build
# Old syntax make ... SUBDIRS=$PWD is still supported
# Setting the environment variable KBUILD_EXTMOD take precedence
@@ -287,7 +297,7 @@ ifeq ($(MAKECMDGOALS),)
endif
export KBUILD_MODULES KBUILD_BUILTIN
-export KBUILD_CHECKSRC KBUILD_SRC KBUILD_EXTMOD
+export KBUILD_CHECKSRC KBUILD_CHECKLOG KBUILD_SRC KBUILD_EXTMOD
# Beautify output
# ---
@@ -1370,6 +1380,7 @@ clean: $(clean-dirs)
$(call cmd,rmfiles)
@find $(if $(KBUILD_EXTMOD), $(KBUILD_EXTMOD), .) $(RCS_FIND_IGNORE) \
\( -name '*.[oas]' -o -name '*.ko' -o -name '.*.cmd' \
+ -o -name '.*.sparse' \
-o -name '*.ko.*' \
-o -name '.*.d' -o -name '.*.tmp' -o -name '*.mod.c' \
-o -name '*.symtypes' -o -name 'modules.order' \
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index bf3e677..45c6004 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -96,14 +96,21 @@ __build: $(if $(KBUILD_BUILTIN),$(builtin-target) $(lib-target) $(extra-y)) \
$(subdir-ym) $(always)
@:
+check_log_file = $(dot-target).$(KBUILD_CHECKLOG).sparse
+
+ifneq ($(KBUILD_CHECKLOG),)
+ check_logging = 2> $(check_log_file)
+endif
+
+cmd_check = $(CHECK) $(CHECKFLAGS) $(c_flags) $< $(check_logging) ;
# Linus' kernel sanity checking tool
ifneq ($(KBUILD_CHECKSRC),0)
ifeq ($(KBUILD_CHECKSRC),2)
quiet_cmd_force_checksrc = CHECK $<
- cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
+ cmd_force_checksrc = $(cmd_check)
else
quiet_cmd_checksrc = CHECK $<
-cmd_checksrc = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
+cmd_checksrc = $(cmd_check)
endif
endif
--
1.9.3
Re: [PATCH] sparse: Add CLOG option for saving warning
On Tue, Jul 8, 2014 at 12:37 AM, Dan Carpenter wrote: > My kernel tree is full of drivers/foo.c.smatch and > drivers/foo.c.smatch-info files... > > It would be nice to add it to .gitignore as well. Actually, ".*" is already in the .gitignore, there for the sparse log files are covered. Chris -- 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] sparse: Add CLOG option for saving warning
On Mon, Jun 30, 2014 at 1:57 AM, Christopher Li wrote: > > Add the CLOG= option in command line to > save the sparse warning into individual log file. > > Typical usage: > > make -j8 C=2 CLOG= Any feed back for this change? I want to clarify that this patch is for the Linux kernel kbuild system, not sparse. Thanks Chris -- 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] sparse: Add CLOG option for saving warning
On Mon, Jul 7, 2014 at 4:22 AM, Sam Ravnborg wrote:
>> > Typical usage:
>> >
>> > make -j8 C=2 CLOG=
>>
> We do not need this kind of special handling of outputs from gcc.
> For sparse you just do a run with C=2 then you have it.
>
> In other words - this looks like overkill for somethign thas is achievable
> with simpler means.
I am aware of the C=2 flag. However, it does not provide consistent result
with "make -jn" flag.
Please consider that the gcc case is different. The kernel source is usually
clean of gcc warnings. If there is an error with gcc, the build process
stops. It is different with sparse. The primary goal of running sparse in
kernel build is to see those warning. Also sparse is a lot noisier than gcc
in the kernel build, so there is a need with logging which is not present
with gcc.
With this patch, here is a normal test procedure for me to see the impact of a
sparse change on kernel build:
$ make -j8 C=2 CLOG=std
# apply sparse change and make sparse
$ make -j8 C=2 CLOG=std-exp
$ find -name ".*.std.sparse" | while read -r file; do diff -du $file
${file/std.sparse/std-exp.
sparse} ; done > /tmp/sparse-diff
Without the CLOG= option, the only way to get similar diff
result is disable "-jn" option. Which make the test process painfully
slow.
I agree if I don't use "make -jn" flags, using C=2 is good enough.
However, what is your suggestion if I do want to use "make -jn" for
sparse checking in the previous mentioned usage case?
Thanks
Chris
--
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: How to fix CHECK warning: testing a 'safe expression' ?
On Thu, Dec 18, 2014 at 6:37 AM, Murali Karicheri wrote:
> if (!ks_pcie) {
> dev_err(dev, "no memory for keystone pcie\n");
> return -ENOMEM;
> }
> pp = &ks_pcie->pp;
>
> /* initialize SerDes Phy if present */
> phy = devm_phy_get(dev, "pcie-phy");
> ===>if (!IS_ERR_OR_NULL(phy)) {
> ret = phy_init(phy);
> if (ret < 0)
> return ret;
>
Hi,
Do you have a smaller stand alone test case which I can reproduce with sparse?
Thanks
Chris
--
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] implement constant-folding in __builtin_bswap*()
On Thu, Aug 11, 2016 at 6:39 PM, Johannes Berg
wrote:
> Since gcc does this, it's apparently valid to write
>
> switch (x) {
> case __builtin_bswap16(12):
>break;
> }
>
> but sparse will flag it as an error today.
>
> The constant folding used to be done in the kernel's htons() and
> friends, but due to gcc bugs that isn't done anymore since
> commit 7322dd755e7d ("byteswap: try to avoid __builtin_constant_p
> gcc bug").
>
> To get rid of the sparse errors on every such instance now, just
> add constant folding to __builtin_bswap*() in sparse.
Sorry for the really late review.
This looks good. I would like to apply it. Can you please add some
test case for the function prototype you introduced, just like the way
kernel use it?
Thanks
Chris
Re: next-20160314 - KASAN breaks 'make C=2' build...
On Tue, Mar 15, 2016 at 4:19 AM, wrote: > > Fedora only packaged 0.5.0, which doesn't include the last few > dozen commits. And they probably won't update until a sparse maintainer > sticks a v0.5.1 tag on it. I suspect that other distros are similar. > > Adding Christopher Li to the recipient list - any thoughts? Yes, I should cut a new release long time ago. Let me just do that. Chris
Re: [PATCH v3] dmaengine: xgene-dma: Fix sparse wannings and coccinelle warnings
On Sun, Apr 26, 2015 at 10:20 PM, Fengguang Wu wrote: >> > >> > drivers/dma/xgene-dma.c:2088:1: sparse: symbol >> > '__UNIQUE_ID_author__COUNTER__' has multiple initializers (originally >> > initialized at drivers/dma/xgene-dma.c:2087) >> > So, I kept only one author here. >> No that is not right, sparse shouldn't have cribbed here. >> >> Fengguang can we get the bot to ignore this please Sorry for the late reply. That looks like the __COUNTER__ macro feature. It has been implemented in sparse git tree already. Chris -- 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: [FAIL 0/5] Static lock analysis
On Wed, Feb 17, 2016 at 1:51 AM, Daniel Wagner wrote: > > Maybe the first 3 patches might be okay to get merged. Yes, the first 3 looks simple and obvious correct. Chris
Re: [tip:locking/core 9/11] include/asm-generic/atomic-instrumented.h:288:24: sparse: cast truncates bits from constant value (100 becomes 0)
On Tue, Mar 13, 2018 at 4:08 AM, Dmitry Vyukov wrote:
> On Tue, Mar 13, 2018 at 1:46 PM, Peter Zijlstra wrote:
>>
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 283 static __always_inline unsigned
>>> >> long
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 284 cmpxchg_size(volatile void
>>> >> *ptr, unsigned long old, unsigned long new, int size)
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 285 {
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 286 switch (size) {
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 287 case 1:
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 @288 return
>>> >> arch_cmpxchg((u8 *)ptr, (u8)old, (u8)new);
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 289 case 2:
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 290 return
>>> >> arch_cmpxchg((u16 *)ptr, (u16)old, (u16)new);
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 291 case 4:
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 292 return
>>> >> arch_cmpxchg((u32 *)ptr, (u32)old, (u32)new);
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 293 case 8:
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 294
>>> >> BUILD_BUG_ON(sizeof(unsigned long) != 8);
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 295 return
>>> >> arch_cmpxchg((u64 *)ptr, (u64)old, (u64)new);
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 296 }
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 297 BUILD_BUG();
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 298 return 0;
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 299 }
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 300
>>
>>> It seems that this is due to this guy:
>>>
>>> static __always_inline int trylock_clear_pending(struct qspinlock *lock)
>>> {
>>> struct __qspinlock *l = (void *)lock;
>>>
>>> return !READ_ONCE(l->locked) &&
>>>(cmpxchg_acquire(&l->locked_pending, _Q_PENDING_VAL,
>>> _Q_LOCKED_VAL) == _Q_PENDING_VAL);
>>> }
>>>
>>> _Q_PENDING_VAL is 0x100. However, locked_pending is 2 bytes. So it
>>> seems that compiler checks all switch cases, this inevitably will lead
>>> to such warnings.
So you are saying cmpxchg_size() was call with a size==2,
however sparse did not do the constant propagation or evaluation
properly. So that it did not eliminate the other branch of size==1
case statement.
Can you do a "sparse -E" on the original file, then save the
result to a new file. That will take care of the pre-processing.
Then run the sparse on the pre-processed file to get a smaller
test case?
Having a smaller test case would make it easier to reproduce
what exactly IR was issued during that warning.
>>> Off the top of my head I can think of the following solution:
>>>
>>> switch (size) {
>>> case 1:
>>> return arch_cmpxchg((u8 *)ptr, (u8)(old * (size !=
>>> 1)), (u8)(new * (size != 1)));
>>> case 2:
>>> return arch_cmpxchg((u16 *)ptr, (u16)(old * (size !=
>>> 2)), (u16)(new * (size != 2)));
>>>
>>> But it's too ugly.
>>
>> Yes agreed, that's horrendous.
Let's not do that. If it is a sparse problem, let's try to fix this sparse.
Chris
Re: Sparse warnings on GENMASK + arm32
On Wed, Jul 26, 2017 at 9:33 AM, Lance Richardson wrote: > Hmm, it seems sparse is incorrectly taking ~0UL to be a 64-bit value > while BITS_PER_LONG is (correctly) evaluated to be 32. > > #define GENMASK(h, l) \ > (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h > What is the sizeof(unsigned long) in ARM 32 bit world? ~0UL has the type of "unsigned long", I assume BITS_PER_LONG is just plain "int"? Using sparse -E should be able to get the expression after the macro expression. The kernel compile invoke sparse directly. That is the assumption that the host gcc has the same type size as the target gcc. That is no longer true if you have cross compiler. If you want to have sparse understand the proper architecture difference, the current practices is using cgcc to handle the architecture specific macros. you can try to invoke the kernel building with: CHECK="cgcc -no-compile". Warning: I haven't try that myself, it might not work as expected. In the long run, I do wish sparse can implement the proper handling of the architecture specific stuff by itself without go through of cgcc. Chris
Re: [git pull] vfs.git part 3
On Fri, Jul 7, 2017 at 8:48 AM, Linus Torvalds wrote: > The releases are done way too seldom to be useful, but that may be > improving. There is one fairly imminent, and it's probably a good idea > to just test the current git tree. Yes guilty of too few releases. We are cutting one release pretty soon. The currently master branch of sparse git repository has gone to RC4 of of release v0.5.1. BTW, the current sparse development has move back to the official sparse repository. The sparse-next is the branch to test the bleeding edge bits. In theory sparse-next can roll back and rewrite history, there is no guarantee it will be clean pull. The master branch will never rebase, it will prove a clean pull. Chris
