Code Review - Migrate include markers to use pragma once

6 views
Skip to first unread message

Davide Libenzi

unread,
Oct 29, 2015, 9:28:26 AM10/29/15
to Akaros
https://github.com/brho/akaros/compare/master...dlibenzi:pragma_once

The following changes since commit 6f3723cd8f883260a78fdf411911d7469464caa5:

  Update file-posix.c utest (2015-10-15 12:07:00 -0400)

are available in the git repository at:

  g...@github.com:dlibenzi/akaros pragma_once

for you to fetch changes up to 6c9904da1e2757de94f11024f8072cac155a1066:

  Migrated Akaros code to use pragma once include file marker (2015-10-29 06:22:45 -0700)

----------------------------------------------------------------
Davide Libenzi (1):
      Migrated Akaros code to use pragma once include file marker

 kern/arch/riscv/arch.h                     | 5 +----
 kern/arch/riscv/atomic.h                   | 5 +----
 kern/arch/riscv/bitmask.h                  | 5 +----
 kern/arch/riscv/colored_page_alloc.h       | 5 +----
 kern/arch/riscv/console.h                  | 5 +----
 kern/arch/riscv/endian.h                   | 5 +----
 kern/arch/riscv/init.h                     | 5 +----
 kern/arch/riscv/kdebug.h                   | 5 +----
 kern/arch/riscv/mmu.h                      | 5 +----
 kern/arch/riscv/pcr.h                      | 5 +----
 kern/arch/riscv/pmap_ops.h                 | 5 +----
 kern/arch/riscv/riscv.h                    | 5 +----
 kern/arch/riscv/ros/arch.h                 | 5 +----
 kern/arch/riscv/ros/membar.h               | 5 +----
 kern/arch/riscv/ros/mmu.h                  | 5 +----
 kern/arch/riscv/ros/syscall.h              | 5 +----
 kern/arch/riscv/ros/trapframe.h            | 5 +----
 kern/arch/riscv/setjmp.h                   | 5 +----
 kern/arch/riscv/smp.h                      | 5 +----
 kern/arch/riscv/softfloat.h                | 5 +----
 kern/arch/riscv/time.h                     | 5 +----
 kern/arch/riscv/topology.h                 | 5 +----
 kern/arch/riscv/trap.h                     | 5 +----
 kern/arch/riscv/types.h                    | 5 +----
 kern/arch/x86/apic.h                       | 5 +----
 kern/arch/x86/arch.h                       | 5 +----
 kern/arch/x86/atomic.h                     | 5 +----
 kern/arch/x86/bitmask.h                    | 5 +----
 kern/arch/x86/bitops.h                     | 5 +----
 kern/arch/x86/colored_page_alloc.h         | 5 +----
 kern/arch/x86/console.h                    | 5 +----
 kern/arch/x86/emulate.h                    | 5 +----
 kern/arch/x86/endian.h                     | 5 +----
 kern/arch/x86/frontend.h                   | 5 +----
 kern/arch/x86/init.h                       | 6 +-----
 kern/arch/x86/io.h                         | 5 +----
 kern/arch/x86/ioapic.h                     | 5 +----
 kern/arch/x86/kbdreg.h                     | 5 +----
 kern/arch/x86/kdebug.h                     | 5 +----
 kern/arch/x86/kpt.h                        | 5 +----
 kern/arch/x86/mmu.h                        | 5 +----
 kern/arch/x86/mptables.h                   | 5 +----
 kern/arch/x86/msr-index.h                  | 5 +----
 kern/arch/x86/pci.h                        | 5 +----
 kern/arch/x86/pci_defs.h                   | 5 +----
 kern/arch/x86/pci_regs.h                   | 5 +----
 kern/arch/x86/perfmon.h                    | 5 +----
 kern/arch/x86/pic.h                        | 5 +----
 kern/arch/x86/pmap.h                       | 5 +----
 kern/arch/x86/pmap_ops.h                   | 5 +----
 kern/arch/x86/ros/arch.h                   | 5 +----
 kern/arch/x86/ros/membar.h                 | 5 +----
 kern/arch/x86/ros/mmu.h                    | 5 ++---
 kern/arch/x86/ros/mmu64.h                  | 5 +----
 kern/arch/x86/ros/syscall.h                | 5 +----
 kern/arch/x86/ros/syscall64.h              | 5 +----
 kern/arch/x86/ros/trapframe.h              | 5 ++---
 kern/arch/x86/ros/trapframe64.h            | 5 +----
 kern/arch/x86/setjmp.h                     | 5 +----
 kern/arch/x86/smp.h                        | 5 +----
 kern/arch/x86/time.h                       | 5 +----
 kern/arch/x86/topology.h                   | 5 +----
 kern/arch/x86/trap.h                       | 5 ++---
 kern/arch/x86/trap64.h                     | 5 +----
 kern/arch/x86/types.h                      | 5 +----
 kern/arch/x86/usb.h                        | 5 +----
 kern/arch/x86/vmm/ept.h                    | 5 +----
 kern/arch/x86/vmm/intel/cpufeature.h       | 4 +---
 kern/arch/x86/vmm/intel/vmx.h              | 5 +----
 kern/arch/x86/vmm/vmm.h                    | 5 +----
 kern/arch/x86/x86.h                        | 5 +----
 kern/drivers/net/bnx2x/bnx2x.h             | 5 +----
 kern/drivers/net/bnx2x/bnx2x_cmn.h         | 5 +----
 kern/drivers/net/bnx2x/bnx2x_dcb.h         | 5 +----
 kern/drivers/net/bnx2x/bnx2x_dump.h        | 4 +---
 kern/drivers/net/bnx2x/bnx2x_fw_defs.h     | 5 +----
 kern/drivers/net/bnx2x/bnx2x_fw_file_hdr.h | 5 +----
 kern/drivers/net/bnx2x/bnx2x_hsi.h         | 5 +----
 kern/drivers/net/bnx2x/bnx2x_init.h        | 7 +------
 kern/drivers/net/bnx2x/bnx2x_init_ops.h    | 4 +---
 kern/drivers/net/bnx2x/bnx2x_link.h        | 5 +----
 kern/drivers/net/bnx2x/bnx2x_mfw_req.h     | 4 +---
 kern/drivers/net/bnx2x/bnx2x_reg.h         | 6 +-----
 kern/drivers/net/bnx2x/bnx2x_sp.h          | 5 +----
 kern/drivers/net/bnx2x/bnx2x_sriov.h       | 4 +---
 kern/drivers/net/bnx2x/bnx2x_stats.h       | 4 +---
 kern/drivers/net/bnx2x/bnx2x_vfpf.h        | 4 +---
 kern/drivers/net/bnx2x/cnic_if.h           | 5 +----
 kern/drivers/net/mlx4/en_port.h            | 6 +-----
 kern/drivers/net/mlx4/fw.h                 | 5 +----
 kern/drivers/net/mlx4/fw_qos.h             | 5 +----
 kern/drivers/net/mlx4/icm.h                | 5 +----
 kern/drivers/net/mlx4/mlx4.h               | 5 +----
 kern/drivers/net/mlx4/mlx4_en.h            | 5 +----
 kern/drivers/net/mlx4/mlx4_stats.h         | 5 +----
 kern/drivers/timers/hpet.h                 | 5 +----
 kern/include/acpi.h                        | 5 +----
 kern/include/alarm.h                       | 5 +----
 kern/include/apipe.h                       | 5 +----
 kern/include/assert.h                      | 5 +----
 kern/include/atomic.h                      | 5 +----
 kern/include/bitmap.h                      | 5 +----
 kern/include/bitmask.h                     | 5 +----
 kern/include/bitops.h                      | 4 +---
 kern/include/bits/netinet.h                | 5 +----
 kern/include/blockdev.h                    | 5 +----
 kern/include/ceq.h                         | 5 +----
 kern/include/colored_caches.h              | 6 +-----
 kern/include/colored_page_alloc.h          | 5 +----
 kern/include/common.h                      | 6 +-----
 kern/include/compat_todo.h                 | 5 +----
 kern/include/console.h                     | 5 +----
 kern/include/coreboot_tables.h             | 4 +---
 kern/include/cpio.h                        | 5 +----
 kern/include/ctype.h                       | 5 +----
 kern/include/debug.h                       | 6 +-----
 kern/include/devalarm.h                    | 5 +----
 kern/include/devfs.h                       | 5 +----
 kern/include/elf.h                         | 5 +----
 kern/include/endian.h                      | 5 +----
 kern/include/env.h                         | 5 +----
 kern/include/err.h                         | 5 +----
 kern/include/error.h                       | 5 +----
 kern/include/event.h                       | 5 +----
 kern/include/ext2fs.h                      | 4 +---
 kern/include/fdtap.h                       | 5 +----
 kern/include/frontend.h                    | 5 +----
 kern/include/hashtable.h                   | 5 +----
 kern/include/ip.h                          | 5 +----
 kern/include/kclock.h                      | 5 +----
 kern/include/kdebug.h                      | 5 +----
 kern/include/kfs.h                         | 5 +----
 kern/include/kmalloc.h                     | 6 +-----
 kern/include/kref.h                        | 5 +----
 kern/include/kstack.h                      | 5 +----
 kern/include/ktest.h                       | 5 +----
 kern/include/kthread.h                     | 5 +----
 kern/include/linker_func.h                 | 5 +----
 kern/include/linux/compat_todo.h           | 5 +----
 kern/include/linux/errno.h                 | 5 +----
 kern/include/linux/mlx4/cmd.h              | 5 +----
 kern/include/linux/mlx4/cq.h               | 5 +----
 kern/include/linux/mlx4/device.h           | 5 +----
 kern/include/linux/mlx4/doorbell.h         | 5 +----
 kern/include/linux/mlx4/driver.h           | 5 +----
 kern/include/linux/mlx4/qp.h               | 5 +----
 kern/include/linux/mlx4/srq.h              | 5 +----
 kern/include/linux/rdma/ib_mad.h           | 5 +----
 kern/include/linux/rdma/ib_smi.h           | 5 +----
 kern/include/linux/rdma/ib_verbs.h         | 5 +----
 kern/include/linux_compat.h                | 5 ++---
 kern/include/list.h                        | 5 +----
 kern/include/manager.h                     | 5 +----
 kern/include/mm.h                          | 5 +----
 kern/include/mmio.h                        | 5 +----
 kern/include/monitor.h                     | 5 +----
 kern/include/multiboot.h                   | 5 +----
 kern/include/ns.h                          | 5 +----
 kern/include/page_alloc.h                  | 6 +-----
 kern/include/pagemap.h                     | 5 +----
 kern/include/pmap.h                        | 5 +----
 kern/include/pool.h                        | 5 +----
 kern/include/process.h                     | 5 +----
 kern/include/radix.h                       | 5 +----
 kern/include/rendez.h                      | 5 +----
 kern/include/ros/atomic.h                  | 5 +----
 kern/include/ros/bcq.h                     | 5 +----
 kern/include/ros/bcq_struct.h              | 5 +----
 kern/include/ros/bits/posix_signum.h       | 5 +----
 kern/include/ros/bits/syscall.h            | 5 +----
 kern/include/ros/ceq.h                     | 5 +----
 kern/include/ros/common.h                  | 5 +----
 kern/include/ros/compiler.h                | 5 +----
 kern/include/ros/errno.h                   | 5 +----
 kern/include/ros/event.h                   | 5 +----
 kern/include/ros/fdtap.h                   | 5 +----
 kern/include/ros/fs.h                      | 5 +----
 kern/include/ros/glibc-asm/ioctl.h         | 5 +----
 kern/include/ros/glibc-asm/ioctls.h        | 5 +----
 kern/include/ros/glibc-asm/socket.h        | 4 +---
 kern/include/ros/glibc-asm/sockios.h       | 5 +----
 kern/include/ros/limits.h                  | 5 +----
 kern/include/ros/memlayout.h               | 5 +----
 kern/include/ros/mman.h                    | 5 +----
 kern/include/ros/procdata.h                | 5 +----
 kern/include/ros/procinfo.h                | 5 +----
 kern/include/ros/resource.h                | 5 +----
 kern/include/ros/ring_buffer.h             | 5 +----
 kern/include/ros/ring_syscall.h            | 5 +----
 kern/include/ros/syscall.h                 | 5 +----
 kern/include/ros/sysevent.h                | 6 +-----
 kern/include/ros/time.h                    | 5 +----
 kern/include/ros/trapframe.h               | 5 ++---
 kern/include/ros/ucq.h                     | 5 +----
 kern/include/ros/virtio_ring.h             | 5 +----
 kern/include/ros/vmm.h                     | 4 +---
 kern/include/rwlock.h                      | 5 +----
 kern/include/schedule.h                    | 5 +----
 kern/include/setjmp.h                      | 5 +----
 kern/include/slab.h                        | 4 +---
 kern/include/smallidpool.h                 | 5 +----
 kern/include/smp.h                         | 5 +----
 kern/include/stab.h                        | 5 +----
 kern/include/stdarg.h                      | 5 +----
 kern/include/stdio.h                       | 5 +----
 kern/include/string.h                      | 5 +----
 kern/include/sys/queue.h                   | 5 +----
 kern/include/sys/types.h                   | 5 +----
 kern/include/sys/uio.h                     | 7 +------
 kern/include/syscall.h                     | 5 +----
 kern/include/taskqueue.h                   | 5 +----
 kern/include/termios.h                     | 5 +----
 kern/include/test_infrastructure.h         | 5 +----
 kern/include/testing.h                     | 5 +----
 kern/include/time.h                        | 4 +---
 kern/include/trace.h                       | 5 +----
 kern/include/trap.h                        | 5 ++---
 kern/include/ucq.h                         | 5 +----
 kern/include/vfs.h                         | 5 +----
 kern/include/zconf.h                       | 5 +----
 kern/include/zlib.h                        | 5 +----
 kern/include/zutil.h                       | 5 +----
 kern/lib/zlib_inflate/inflate.h            | 4 +---
 kern/lib/zlib_inflate/inftrees.h           | 4 +---
 kern/lib/zlib_inflate/infutil.h            | 5 +----
 225 files changed, 231 insertions(+), 891 deletions(-)

ron minnich

unread,
Oct 29, 2015, 11:22:35 AM10/29/15
to Akaros
First, thanks for figuring out how to put the URL in the review, that makes it tons easier for me.

Second, this is a huge improvement.

It's a good way to deal with .h files that include .h files. I'd still most prefer that we just don't have .h files include .h files*, which is the real problem, but we're not going to fix that anytime soon ;-), so:

LGTM

thanks!

ron
*Another OS Project I worked with for years did follow the rule: .h doesn't include .h. So I know it's doable :-)

--
You received this message because you are subscribed to the Google Groups "Akaros" group.
To unsubscribe from this group and stop receiving emails from it, send an email to akaros+un...@googlegroups.com.
To post to this group, send email to aka...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Barret Rhoden

unread,
Nov 10, 2015, 11:44:13 AM11/10/15
to aka...@googlegroups.com
Sorry for the hassle, but can you rerun this script from the latest
origin/staging? There's been a bunch of header changes, esp with the
VMM stuff, that prevents this from applying cleanly.

Thanks,

Barret


On 2015-10-29 at 06:28 "'Davide Libenzi' via Akaros"

Davide Libenzi

unread,
Nov 10, 2015, 11:48:16 AM11/10/15
to Akaros
Sure. Damn Ron, you can't even add code w/out changing header files! ☺
It will be in a new branch though ...


Davide Libenzi

unread,
Nov 10, 2015, 12:02:29 PM11/10/15
to Akaros
Same branch. Try how it looks now ...

Barret Rhoden

unread,
Nov 10, 2015, 2:07:49 PM11/10/15
to aka...@googlegroups.com
On 2015-11-10 at 09:02 "'Davide Libenzi' via Akaros"
<aka...@googlegroups.com> wrote:
> Same branch. Try how it looks now ...

Thanks. This merges, but there are a couple issues.

A minor one is that the headers in user/ and tests/ weren't included.

The big one is that this changes the kernel headers and actually
breaks the compilation. The issue is stuff like this:

kern/arch/x86/ros/syscall64.h:#error "Do not include include
ros/arch/syscall64.h directly"

this is what pops up:

#ifndef ROS_INC_ARCH_SYSCALL_H
#error "Do not include include ros/arch/syscall64.h directly"
#endif

since we no longer define ROS_INC_ARCH_SYSCALL_H

There's about 10 of these throughout the kernel and user space
headers. (git grep "Do not include").

A couple options:
1) Keep the #error, but go into the header that should have been
included and #define something like INCLUDED_INC_SYSCALL
2) Just remove all of those checks. They aren't really a big deal
anyways.

Either option works for me, or whatever else people suggest. The
cleanest way to do it is to make one commit that makes the change
(option 1, 2, etc), then do the big perl script change in another
commit.

Barret

Davide Libenzi

unread,
Nov 10, 2015, 2:10:38 PM11/10/15
to Akaros
It was compiling fine for me.
Yes, I had to fix manually what you mention, but again, it is building fine, my branch.
Let me triple check ...


Davide Libenzi

unread,
Nov 10, 2015, 2:12:26 PM11/10/15
to Akaros
Yep, it does.

Davide Libenzi

unread,
Nov 10, 2015, 2:17:32 PM11/10/15
to Akaros
Adding /tests and /user in there as well ...

Davide Libenzi

unread,
Nov 10, 2015, 2:19:51 PM11/10/15
to Akaros
Done.
That branch builds for me, let me know otherwise.
Yes, even in parlib, I had to manually fix one of those issues that you mentioned.
There were a few on kern/.

Message has been deleted

Barret Rhoden

unread,
Nov 10, 2015, 3:09:44 PM11/10/15
to aka...@googlegroups.com
On 2015-11-10 at 11:12 "'Davide Libenzi' via Akaros"
<aka...@googlegroups.com> wrote:
> Yep, it does.

The thing that wasn't compiling was the toolchain itself. I tried to
do that since some of the kern/include/ros/ header files were changed.
I'll try your latest.

Thanks,
Barret

Barret Rhoden

unread,
Nov 10, 2015, 3:18:48 PM11/10/15
to aka...@googlegroups.com
On 2015-11-10 at 11:19 "'Davide Libenzi' via Akaros"
<aka...@googlegroups.com> wrote:
> Done.
> That branch builds for me, let me know otherwise.
> Yes, even in parlib, I had to manually fix one of those issues that
> you mentioned.
> There were a few on kern/.

Maybe I'm missing it, but it still doesn't build for me. I'm looking
at commit:

21a42fcec7a1 ("Migrated Akaros code to use pragma once include file
marker")

It has the user and tests changes, but it still has the #error stuff.
e.g.:

diff --git a/kern/arch/x86/ros/syscall64.h b/kern/arch/x86/ros/syscall64.h
index 4ec8e85b8bf9..7b63ca745b7e 100644
--- a/kern/arch/x86/ros/syscall64.h
+++ b/kern/arch/x86/ros/syscall64.h
@@ -1,5 +1,4 @@
-#ifndef ROS_INC_ARCH_SYSCALL64_H
-#define ROS_INC_ARCH_SYSCALL64_H
+#pragma once

#ifndef ROS_INC_ARCH_SYSCALL_H
#error "Do not include include ros/arch/syscall64.h directly"


Can you remove or rename the #define for those cases in a single
commit, then run the perl script for another commit?

Thanks,

Barret

Davide Libenzi

unread,
Nov 10, 2015, 3:22:07 PM11/10/15
to Akaros
Did the commit apply correctly?
Yes, the #error is still there, but the ones including it, are correctly defining what that file wants.



Barret

Kevin Klues

unread,
Nov 10, 2015, 4:15:50 PM11/10/15
to aka...@googlegroups.com
Does everything build cleanly for you Davide if you run:

make xcc-upgrade-from-scratch?

it sounds like this is where it is breaking for barret.

On Tue, Nov 10, 2015 at 3:22 PM, 'Davide Libenzi' via Akaros
--
~Kevin

Barret Rhoden

unread,
Nov 10, 2015, 4:48:58 PM11/10/15
to aka...@googlegroups.com
On 2015-11-10 at 12:22 "'Davide Libenzi' via Akaros"
<aka...@googlegroups.com> wrote:
> Did the commit apply correctly?
> Yes, the #error is still there, but the ones including it, are
> correctly defining what that file wants.

i'll try it again, in case i applied it incorrectly or something.

barret

Barret Rhoden

unread,
Nov 10, 2015, 4:53:34 PM11/10/15
to aka...@googlegroups.com
On 2015-11-10 at 12:22 "'Davide Libenzi' via Akaros"
<aka...@googlegroups.com> wrote:
> Did the commit apply correctly?
> Yes, the #error is still there, but the ones including it, are
> correctly defining what that file wants.

looks like it still doesn't work for me. for example:

$ cd tools/compilers/gcc-glibc/
$ make x86_64

(eventual errors like this):

/home/brho/akaros/ros-gcc-glibc/install-x86_64-ros-gcc/x86_64-ucb-akaros/sysroot/usr/include/ros/arch/syscall64.h:4:2:
error: #error "Do not include include ros/arch/syscall64.h directly"
#error "Do not include include ros/arch/syscall64.h directly"

looking at kern/arch/x86/ros/syscall64.h:

#ifndef ROS_INC_ARCH_SYSCALL_H
#error "Do not include include ros/arch/syscall64.h directly"
#endif


then looking for a #define of INC_ARCH_SYSCALL_H:

$ git grep ROS_INC_ARCH_SYSCALL_H
kern/arch/x86/ros/syscall64.h:#ifndef ROS_INC_ARCH_SYSCALL_H

that appears in just one location, and is never #defined. maybe just
that one is busted?

barret


Davide Libenzi

unread,
Nov 10, 2015, 5:28:37 PM11/10/15
to Akaros
Oh, wait, I did not run the xcc saga ...


Davide Libenzi

unread,
Nov 10, 2015, 5:29:16 PM11/10/15
to Akaros
Let me check that (and wait a little, since xcc is longish) ...

Davide Libenzi

unread,
Nov 10, 2015, 5:53:25 PM11/10/15
to Akaros
OK, now even xcc saga is building. It was just one fix needed.

Barret Rhoden

unread,
Nov 11, 2015, 11:38:59 AM11/11/15
to aka...@googlegroups.com
Thanks, merged to staging at a759dfde3f64..1ed63f9f8e25 (from, to]

You can see the entire diff with 'git diff' or at
https://github.com/brho/akaros/compare/a759dfde3f64...1ed63f9f8e25



On 2015-11-10 at 14:53 "'Davide Libenzi' via Akaros"

Kevin Klues

unread,
Nov 11, 2015, 11:45:58 AM11/11/15
to aka...@googlegroups.com
To be clear to others following this, the rules from here forward are
to use #pragma once in new header files, instead of the usual:

#ifndef THING
#define THING
....
#endif
--
~Kevin
--
~Kevin
Reply all
Reply to author
Forward
0 new messages