6611 NULL pointer constant issues in uts/i86pc

Review Request #131 — Created Feb. 6, 2016 and updated

risto3
illumos-gate
5218, 6611
37518e7...
general
6611 NULL pointer constant issues in uts/i86pc

updated review since cstyle updates and rebase to a recent upstream (illumos-gate/master).

mail_msg and wsdiff (of cumulated build of il_6609, il_6610 and il_6611... respectively r/129, r/130 and r/131)
shows no significant differences (file attached).

NOTE: ELF .SUNW_dof difference detected.
NOTE: ELF .SUNW_dof difference detected.
NOTE: ELF .SUNW_dof difference detected.
NOTE: ELF .SUNW_dof difference detected.
NOTE: ELF .SUNW_dof difference detected.
NOTE: ELF .SUNW_dof difference detected.
NOTE: ELF .SUNW_dof difference detected.
NOTE: ELF .SUNW_dof difference detected.
NOTE: ELF .rodata difference detected.
NOTE: ELF .rodata difference detected.
NOTE: ELF .SUNW_dof difference detected.
NOTE: ELF .rodata difference detected.
NOTE: ELF .rodata difference detected.
NOTE: ELF .rodata difference detected.
NOTE: ELF .data difference detected.
NOTE: ELF .data difference detected.
NOTE: ELF .text difference detected.
NOTE: ELF .data difference detected.
NOTE: ELF .data difference detected.
NOTE: ASCII difference detected.

the SUNW_dof, .[ro]data and ASCII differences are related to the branch name and datetime stamps.
the only code difference (.text) is due to two source code lines being suppressed as detailed below
concerning usr/src/uts/i86pc/io/dr/dr.c

Although I had long run a version of the gate with these mods, I have not done so recently
waiting for a perl64 enabled patchset to push since I run basically on omnios-bloody.

platform/i86pc/kernel/drv/amd64/dr
NOTE: ELF .text difference detected.
============================================================================================
3733c3733
<     dr_dev_status+0x3a8:          41 b8 fb 08 00 00  movl   $0x8fb,%r8d
---
>     dr_dev_status+0x3a8:          41 b8 f9 08 00 00  movl   $0x8f9,%r8d
3749c3749
<     dr_dev_status+0x3e6:          41 b8 03 09 00 00  movl   $0x903,%r8d
---
>     dr_dev_status+0x3e6:          41 b8 01 09 00 00  movl   $0x901,%r8d
============================================================================================
 6547               .LBB68:
2294:../../i86pc/io/dr/dr.c ****            PR_ALL("%s: alignment: sz=0x%lx dstat32p=0x%p\n",
 6548                   .loc 1 2294 0
 6549 340d F68580FE         testb   $1, -384(%rbp)
 6549      FFFF01
2297:../../i86pc/io/dr/dr.c ****            rv = EINVAL;
 6550                   .loc 1 2297 0
 6551 3414 498D7508         leaq    8(%r13), %rsi
>6552 3418 41B8F908         movl    $2297, %r8d
 6552      0000
2294:../../i86pc/io/dr/dr.c ****            PR_ALL("%s: alignment: sz=0x%lx dstat32p=0x%p\n",
 6553                   .loc 1 2294 0
 6554 341e 753C             jne .L594
2303:../../i86pc/io/dr/dr.c ****            ((caddr_t)dstatp) + sz) {
 6555                   .loc 1 2303 0
 6556 3420 49638424         movslq  304(%r12),%rax
 6556      30010000 
 6557 3428 488B8D80         movq    -384(%rbp), %rcx
 6557      FEFFFF
 6558 342f 4869C084         imulq   $1156, %rax, %rax
 6558      040000
 6559 3436 488D9401         leaq    304(%rcx,%rax), %rdx
 6559      30010000 
 6560 343e 488B8D58         movq    -424(%rbp), %rcx
 6560      FEFFFF
 6561 3445 498D040C         leaq    (%r12,%rcx), %rax
 6562 3449 4839C2           cmpq    %rax, %rdx
 6563 344c 0F866602         jbe .L575
 6563      0000
2305:../../i86pc/io/dr/dr.c ****            rv = EINVAL;
 6564                   .loc 1 2305 0
 6565 3452 498D7508         leaq    8(%r13), %rsi
>6566 3456 41B80109         movl    $2305, %r8d
 6566      0000

=============================================================================================
5159c5159
<     dr_dev_found+0x4c:            b9 09 0b 00 00     movl   $0xb09,%ecx
---
>     dr_dev_found+0x4c:            b9 07 0b 00 00     movl   $0xb07,%ecx
=============================================================================================
2818:../../i86pc/io/dr/dr.c ****        /*
2819:../../i86pc/io/dr/dr.c ****         * this should not happen.  When it does, it indicates
2820:../../i86pc/io/dr/dr.c ****         * a missmatch in devices supported by the drmach layer
2821:../../i86pc/io/dr/dr.c ****         * vs devices supported by this layer.
2822:../../i86pc/io/dr/dr.c ****         */
2823:../../i86pc/io/dr/dr.c ****        return (DR_INTERNAL_ERROR());
 8754                   .loc 1 2823 0
 8755 4815 488B1500         movq    dr_ie_fmt(%rip), %rdx
 8755      000000
>8756 481c B9070B00         movl    $2823, %ecx
 8756      00
 8757 4821 BE010000         movl    $1, %esi
 8757      00
 8758 4826 BF010000         movl    $1, %edi
 8758      00
 8759 482b 31C0             xorl    %eax, %eax
 8760               .LVL961:
 8761 482d E8000000         call    drerr_new
 8761      00

as can be seen by the '>', the address is correct, the line number is simply two less
since the cstyle update lines 1711 and 1936 in dr.c

Loading file attachments...

  • 0
  • 0
  • 0
  • 2
  • 2
Description From Last Updated
tsoome
  1. 
      
  2. usr/src/uts/i86pc/io/cbe.c (Diff revision 1)
     
     

    since kernel is built C99, perhaps its better to use:
    -static ddi_softint_hdl_impl_t cbe_low_hdl =
    - {0, NULL, NULL, NULL, 0, NULL, NULL, NULL};
    +static ddi_softint_hdl_impl_t cbe_low_hdl = {
    + .ih_dip = NULL,
    + .ih_pri = 0,
    + .ih_rwlock = NULL,
    + .ih_pending = NULL,
    + .ih_cb_func = NULL,
    + .ih_cb_arg1 = NULL,
    + .ih_cb_arg2 = NULL,
    + .ih_private = NULL
    +};

    1. I like that, but I should mention there are actually some other cases with
      huge nested records.

      What about a constant or macro in the header such as NULL_DDI_SOFTINT_HDL defined thus?

      Personnally I'd like to see more use of these types of "initialisers"...

      In particular to define the function pointers and NULL function pointer constants
      with all the gang as is the case here.

      See usr/src/uts/common/io/avintr.c in il_6615
      (?? somehow seems missing, it's in github in il_5218-20151231, I'll need to update 6615 I guess)

    2. yes, one thing is about common NULL initializers and macro seems quite good idea. also in some other cases there were much larger structs/lists, re-making them all feels like going past the target of this work and perhaps separate issue should be opened instead:)

    3. my error, avintr.c is treated in il_6614.

      may I propose, then, to keep the relative ordering for now with a separate issue for the restructure?

    4. Yes, I think it would be the best not to overcomplicate this work.

  3. 
      
tsoome
  1. 
      
  2. usr/src/uts/i86pc/os/machdep.c (Diff revision 1)
     
     

    same C99 comment here:)

  3. 
      
risto3
Review request changed

Testing Done:

  +

updated review since cstyle updates and rebase to a recent upstream (illumos-gate/master).

  +
  +

mail_msg and wsdiff (of cumulated build of il_6609, il_6610 and il_6611... respectively r/129, r/130 and r/131)

  + shows no significant differences (file attached).

  +
  +

NOTE: ELF .SUNW_dof difference detected.

  + NOTE: ELF .SUNW_dof difference detected.
  + NOTE: ELF .SUNW_dof difference detected.
  + NOTE: ELF .SUNW_dof difference detected.
  + NOTE: ELF .SUNW_dof difference detected.
  + NOTE: ELF .SUNW_dof difference detected.
  + NOTE: ELF .SUNW_dof difference detected.
  + NOTE: ELF .SUNW_dof difference detected.
  + NOTE: ELF .rodata difference detected.
  + NOTE: ELF .rodata difference detected.
  + NOTE: ELF .SUNW_dof difference detected.
  + NOTE: ELF .rodata difference detected.
  + NOTE: ELF .rodata difference detected.
  + NOTE: ELF .rodata difference detected.
  + NOTE: ELF .data difference detected.
  + NOTE: ELF .data difference detected.
  + NOTE: ELF .text difference detected.
  + NOTE: ELF .data difference detected.
  + NOTE: ELF .data difference detected.
  + NOTE: ASCII difference detected.

  +
  +

the SUNW_dof, .[ro]data and ASCII differences are related to the branch name and datetime stamps.

  + the only code difference (.text) is due to two source code lines being suppressed as detailed below
  + concerning usr/src/uts/i86pc/io/dr/dr.c

  +
  +

Although I had long run a version of the gate with these mods, I have not done so recently

  + waiting for a perl64 enabled patchset to push since I run basically on omnios-bloody.

  +
  +

  +
  +
platform/i86pc/kernel/drv/amd64/dr
  +
NOTE: ELF .text difference detected.
  +
============================================================================================
  +
3733c3733
  +
<     dr_dev_status+0x3a8:          41 b8 fb 08 00 00  movl   $0x8fb,%r8d
  +
---
  +
>     dr_dev_status+0x3a8:          41 b8 f9 08 00 00  movl   $0x8f9,%r8d
  +
3749c3749
  +
<     dr_dev_status+0x3e6:          41 b8 03 09 00 00  movl   $0x903,%r8d
  +
---
  +
>     dr_dev_status+0x3e6:          41 b8 01 09 00 00  movl   $0x901,%r8d
  +
============================================================================================
  +
 6547               .LBB68:
  +
2294:../../i86pc/io/dr/dr.c ****            PR_ALL("%s: alignment: sz=0x%lx dstat32p=0x%p\n",
  +
 6548                   .loc 1 2294 0
  +
 6549 340d F68580FE         testb   $1, -384(%rbp)
  +
 6549      FFFF01
  +
2297:../../i86pc/io/dr/dr.c ****            rv = EINVAL;
  +
 6550                   .loc 1 2297 0
  +
 6551 3414 498D7508         leaq    8(%r13), %rsi
  +
>6552 3418 41B8F908         movl    $2297, %r8d
  +
 6552      0000
  +
2294:../../i86pc/io/dr/dr.c ****            PR_ALL("%s: alignment: sz=0x%lx dstat32p=0x%p\n",
  +
 6553                   .loc 1 2294 0
  +
 6554 341e 753C             jne .L594
  +
2303:../../i86pc/io/dr/dr.c ****            ((caddr_t)dstatp) + sz) {
  +
 6555                   .loc 1 2303 0
  +
 6556 3420 49638424         movslq  304(%r12),%rax
  +
 6556      30010000 
  +
 6557 3428 488B8D80         movq    -384(%rbp), %rcx
  +
 6557      FEFFFF
  +
 6558 342f 4869C084         imulq   $1156, %rax, %rax
  +
 6558      040000
  +
 6559 3436 488D9401         leaq    304(%rcx,%rax), %rdx
  +
 6559      30010000 
  +
 6560 343e 488B8D58         movq    -424(%rbp), %rcx
  +
 6560      FEFFFF
  +
 6561 3445 498D040C         leaq    (%r12,%rcx), %rax
  +
 6562 3449 4839C2           cmpq    %rax, %rdx
  +
 6563 344c 0F866602         jbe .L575
  +
 6563      0000
  +
2305:../../i86pc/io/dr/dr.c ****            rv = EINVAL;
  +
 6564                   .loc 1 2305 0
  +
 6565 3452 498D7508         leaq    8(%r13), %rsi
  +
>6566 3456 41B80109         movl    $2305, %r8d
  +
 6566      0000
  +
  +
=============================================================================================
  +
5159c5159
  +
<     dr_dev_found+0x4c:            b9 09 0b 00 00     movl   $0xb09,%ecx
  +
---
  +
>     dr_dev_found+0x4c:            b9 07 0b 00 00     movl   $0xb07,%ecx
  +
=============================================================================================
  +
2818:../../i86pc/io/dr/dr.c ****        /*
  +
2819:../../i86pc/io/dr/dr.c ****         * this should not happen.  When it does, it indicates
  +
2820:../../i86pc/io/dr/dr.c ****         * a missmatch in devices supported by the drmach layer
  +
2821:../../i86pc/io/dr/dr.c ****         * vs devices supported by this layer.
  +
2822:../../i86pc/io/dr/dr.c ****         */
  +
2823:../../i86pc/io/dr/dr.c ****        return (DR_INTERNAL_ERROR());
  +
 8754                   .loc 1 2823 0
  +
 8755 4815 488B1500         movq    dr_ie_fmt(%rip), %rdx
  +
 8755      000000
  +
>8756 481c B9070B00         movl    $2823, %ecx
  +
 8756      00
  +
 8757 4821 BE010000         movl    $1, %esi
  +
 8757      00
  +
 8758 4826 BF010000         movl    $1, %edi
  +
 8758      00
  +
 8759 482b 31C0             xorl    %eax, %eax
  +
 8760               .LVL961:
  +
 8761 482d E8000000         call    drerr_new
  +
 8761      00
  +
  +

as can be seen by the '>', the address is correct, the line number is simply two less

  + since the cstyle update lines 1711 and 1936 in dr.c

Commit:

-70db7d75d1b7c7fac2a13eca480c7661b31873a0
+37518e73b5b1c2aa114ae386a454966504dda661

Diff:

Revision 2 (+87 -81)

Show changes

Added Files:

tsoome
  1. Ship It!
  2. 
      
jeffpc
  1. Ship It!
  2. 
      
kmays
  1. Ship It!
  2. 
      
Loading...