Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi-channel file descriptors - cause core dump (4.200) #2931

Closed
agrobman opened this issue May 7, 2021 · 15 comments
Closed

Multi-channel file descriptors - cause core dump (4.200) #2931

agrobman opened this issue May 7, 2021 · 15 comments
Labels
area: wrong runtime result Issue involves an incorrect runtine result from Verilated model resolution: fixed Closed; fixed

Comments

@agrobman
Copy link

agrobman commented May 7, 2021

I was trying to write simultaneously to a file and sim log, but seems this does not work on 4.102 and causes core dump on 4.200.

% obj_dir/Vour
Hello World
Segmentation fault (core dumped)

the code is modified example from manual:

  module our;
  integer fd;

     initial begin 
        $display("Hello World"); 
        fd = $fopen("kuku.txt");
        fd = fd | 1;
        $fwrite(fd, "writing to kuku.txt\n");
        $fclose(fd);
        $finish; 
     end
  endmodule

with following CPP wrapper:

#include "Vour.h"
 #include "verilated.h"
 int main(int argc, char** argv, char** env) {
     VerilatedContext* contextp = new VerilatedContext;
     contextp->commandArgs(argc, argv);
     Vour* top = new Vour{contextp};
     while (!contextp->gotFinish()) { top->eval(); }
     delete top;
     delete contextp;
     return 0;
 }

It does write file if I comment out the line:

fd = fd | 1;

Change log shows that multichannel file descriptors were implemented back in 4.038 (?)
On 4.102 the "|1" seems just ignored - the $fwrite writes only to file, not to screen/log

what's wrong?

@agrobman agrobman added the new New issue not seen by maintainers label May 7, 2021
@wsnyder
Copy link
Member

wsnyder commented May 7, 2021

This should work, note t_display_mcd.v and t_sys_file_basic_mcd.v test this. Perhaps you could see what makes these different and provide a patch to fix whatever's up (likely a bug in verilated.cpp)?

@agrobman
Copy link
Author

agrobman commented May 7, 2021

@wsnyder, I'm not C/C++ programmer you talk to me Chinese, I don't know ... I'm EE, know only verilog..

where should I see the mentioned files?

@agrobman
Copy link
Author

agrobman commented May 7, 2021

Wilson, seems I miss something ..

the Verilog spec says that MCD should have bit 31 clear , t_display_mcd.v test uses 32'h8000_0001 and 32'h8000_0003 constants in $fwrite calls. (basicaly this test does no use MCD, but just checks predefined STDIN, STDOUT and STDERR predefined file descriptors)

the 2nd test just opens and closes files, but does not try to write to them to prove that the same data can be written to multiple files with single $fwrite statement and also there is no try to write to a file and STDOUT/log as my code tries to do...

@agrobman
Copy link
Author

agrobman commented May 7, 2021

taking 1st complain about 2nd test back, I missed task 2, but there is no fd | 1 test - write to log and a file...

@agrobman
Copy link
Author

agrobman commented May 7, 2021

update : writing to two files works:

module our;
 integer fd, fd1, all;

    initial begin 
       $display("%m:Hello World"); 
       fd = $fopen("kuku.txt");
       fd1 = $fopen("kuku1.txt");
       $display("fd=%h, fd1=%h",fd, fd1);
       all = fd | fd1;
       $fwrite(all, "writing to kuku.txt\n");
       $fclose(fd);
       $fclose(fd1);
       $finish; 
    end
 endmodule

@stephenry
Copy link
Contributor

stephenry commented May 7, 2021

I did the MCD stuff last year. I'll take a look at it over the weekend. It's been a while but I can't recall if the MCD file descriptor treats bits 1/2 as STDOUT/STDERR. Perhaps it should, perhaps it shouldn't; will need to review the spec.

@agrobman
Copy link
Author

agrobman commented May 7, 2021

update : segmentation fault was due my mistake : I used "ORed" descriptor in $fclose.

Now the code should be as

module our;
 integer fd, fd1, all;

    initial begin 
       $display("%m:Hello World"); 
       fd = $fopen("kuku.txt");
//        fd1 = $fopen("kuku1.txt");
       $display("fd=%h, fd1=%h",fd, fd1);
       all = fd | 1;
       $fwrite(all, "writing to kuku.txt\n");
       $fclose(fd);
//        $fclose(fd1);
       $finish; 
    end
 endmodule

log/results:

% obj_dir/Vour
our:Hello World
fd=40000000, fd1=00000000
- our.v:13: Verilog $finish

As you can see nothing was printed on screen, but kuku.txt is written:

% cat kuku.txt
writing to kuku.txt


@agrobman
Copy link
Author

agrobman commented May 7, 2021

BTW, may be it's worth to check $fclose argument in simulator to avoid this segmentation fault ... - issue a runtime error or warning ...

@wsnyder
Copy link
Member

wsnyder commented May 8, 2021

$fclose is supposed to take a MCD, so I think your program was reasonable but there's probably a bug there, e.g. maybe it closes stdout? - @stephenry sounds like you offered to look when you get a chance - thanks!

@agrobman
Copy link
Author

@stephenry , any updates for the issue? Do you confirm the problem?

@stephenry
Copy link
Contributor

I did take a look at this over the weekend. Unfortunately, the only machine I have available to me at the moment is my M1. Verilator's regression environment does not appear to successfully link on this architecture yet and although I traced the issue down to the (recently updated) archival step in verilated.mk to produce the __ALL.a, I wasn't able to resolve the issue.

If I was to hazard a guess, the crash is caused by the fact that the "or 1" here, causes the 0'th entry in the MCD descriptor to become set (effectively bypassing the code for fopen which would create the file descriptor in that location). When attempting to write to this descriptor we end up dereferencing a null pointer. After initial inspection, I don't think I hardwired the 0th entry in the MCD descriptor to stdout.

I'll probably need to dig out my Linux box and fix it from there. Is this currently blocking you? Should be relatively easy to work around for the moment.

@agrobman
Copy link
Author

I'm porting code from Xcelium to verilator. this is one of many problems on my way. Will be nice to get it fix sooner than later.
Remember fd | 1 does not work too!

@agrobman
Copy link
Author

Any good news for me?

@wsnyder wsnyder added area: runtime result status: ready Issue is ready for someone to fix; then goes to 'status: assigned' resolution: fixed Closed; fixed and removed new New issue not seen by maintainers status: ready Issue is ready for someone to fix; then goes to 'status: assigned' labels Jun 4, 2021
@wsnyder wsnyder closed this as completed Jun 6, 2021
@agrobman
Copy link
Author

agrobman commented Jun 7, 2021

Hi Wilson,

Does this fix my original problem - writing STDOUT and a file simultaneously ?

@wsnyder
Copy link
Member

wsnyder commented Jun 7, 2021

Yes.

tklam pushed a commit to tklam/verilator that referenced this issue Jun 8, 2021
commit 6fd48da
Author: Wilson Snyder <wsnyder@wsnyder.org>
Date:   Mon Jun 7 07:47:15 2021 -0400

    Untabify cmakefiles. No functional change.

commit b976b8d
Author: Wilson Snyder <wsnyder@wsnyder.org>
Date:   Sun Jun 6 22:09:30 2021 -0400

    Fix slowdown in elaboration (verilator#2911).

commit c238d3d
Author: github action <action@example.com>
Date:   Sun Jun 6 23:57:41 2021 +0000

    Apply clang-format

commit 0edf1f0
Author: Geza Lore <gezalore@gmail.com>
Date:   Mon Jun 7 00:56:30 2021 +0100

    Add ccache-report target to standard Makefile (verilator#3011)

    Using the standard model Makefile, when in addition to an explicit
    target, the target 'ccache-report' is also given, a summary of ccache
    hits/misses during this invocation of 'make' will be prited at the end
    of the build.

commit 31bb73e
Author: Wilson Snyder <wsnyder@wsnyder.org>
Date:   Sun Jun 6 19:32:48 2021 -0400

    Fix MCD close also closing stdout (verilator#2931).

commit 8f2e4f6
Author: Wilson Snyder <wsnyder@wsnyder.org>
Date:   Sun Jun 6 10:32:50 2021 -0400

    Fix clang warning.

commit 2158a45
Author: Wilson Snyder <wsnyder@wsnyder.org>
Date:   Sun Jun 6 10:27:44 2021 -0400

    Tests: Reenable 18.04 MT (verilator#2963).

commit 1e89392
Author: Wilson Snyder <wsnyder@wsnyder.org>
Date:   Sun Jun 6 10:27:01 2021 -0400

    Add --expand-limit argument (verilator#3005).

commit b0c1ac7
Author: Martin Schmidt <martin.schmidt@epita.fr>
Date:   Sun Jun 6 15:27:44 2021 +0200

    Add support of --trace-structs parameter for CMake (verilator#2986)

commit 1f19e8e
Author: Wilson Snyder <wsnyder@wsnyder.org>
Date:   Sun Jun 6 09:17:56 2021 -0400

    Tests: Add test case for verilator#2895.

commit 258d9ad
Author: Geza Lore <gezalore@gmail.com>
Date:   Sat Jun 5 17:40:56 2021 +0100

    Aid IDE code linking in V3AstNodes.h (verilator#3009)

commit fa06357
Author: Miodrag Milanović <mmicko@gmail.com>
Date:   Fri Jun 4 18:04:55 2021 +0200

    Fix Makefiles to support Windows EXEEXT usage (verilator#3008).

commit eea7e1b
Author: Geza Lore <gezalore@gmail.com>
Date:   Fri Jun 4 15:00:13 2021 +0100

    Do not emit leading spaces on blank lines (verilator#3007)

commit fb561d9
Author: Wilson Snyder <wsnyder@wsnyder.org>
Date:   Thu Jun 3 21:19:11 2021 -0400

    Commentary (verilator#2996)

commit 9f5eb8f
Author: Wilson Snyder <wsnyder@wsnyder.org>
Date:   Thu Jun 3 21:15:42 2021 -0400

    Commentary

commit 1f331bd
Author: Julien Margetts <56540603+margej@users.noreply.github.com>
Date:   Tue Jun 1 14:01:18 2021 +0100

    Fix part select issues in LATCH warning. (verilator#2948) (verilator#2938)

commit 2143bcf
Author: Wilson Snyder <wsnyder@wsnyder.org>
Date:   Mon May 31 22:46:41 2021 -0400

    Fix constant function calls with uninit value (verilator#2995).

commit e1f9fff
Author: Geza Lore <gezalore@gmail.com>
Date:   Mon May 31 13:40:22 2021 +0100

    Fix --protect-ids when using SV classes (verilator#2994)

    A few names were incorrectly mangled, which made --protect-ids produce
    invalid output when certain SV class constructs were uses. Now fixed and
    added a few extra tests to catch this.

commit e4dcbb2
Author: Pieter Kapsenberg <pieter.kapsenberg@gmail.com>
Date:   Sun May 30 17:19:35 2021 -0700

    Fix unused variable warnings (verilator#2991)

commit f4c1a7e
Author: Geza Lore <gezalore@gmail.com>
Date:   Sat May 29 23:46:15 2021 +0100

    Emit: Fix indent tracking when // inside string literal (verilator#2990)

    // was so far unconditionally treated as comment.
    c443e22 introduced a string literal in
    the output that contained a http:// url, which broke the indent
    tracking. No functional change intended

commit ef9f477
Author: Geza Lore <gezalore@gmail.com>
Date:   Sat May 22 18:50:55 2021 +0100

    Make _ctror_var_reset and _configure_coverage static. (verilator#2977)

    Another step towards verilator#2958/verilator#2140. Make the mentioned generated functions
    static for modules (but not for classes).

commit 2dd5ef5
Author: Geza Lore <gezalore@gmail.com>
Date:   Sat May 22 12:27:32 2021 +0100

    Internals: Move --coverage and --savable check out of V3EmitC (verilator#2976)

commit a43eba7
Author: github action <action@example.com>
Date:   Sat May 22 10:14:07 2021 +0000

    Apply clang-format

commit 61c9866
Author: Geza Lore <gezalore@gmail.com>
Date:   Sat May 22 11:13:02 2021 +0100

    Internals: Generate ASTGEN_SUPER_* macros instead of expanding. (verilator#2975)

    astgen now generates ASTGEN_SUPER_* macros, instead of expanding the
    ASTGEN_SUPER itself. This means that V3AstNodes.h itself can be included
    in V3Ast.h, which helps IDEs (namely CLion) find definitions in
    V3AstNodes.h a lot better. Albeit this is a little bit more boilerplate,
    writing constructors of Ast nodes should be a lot rarer than trying to
    find their definitions, so hopefully this is an improvement overall.
    astgen will error if the developer calls the wrong superclass
    constructor.

commit 6378255
Author: Wilson Snyder <wsnyder@wsnyder.org>
Date:   Fri May 21 20:47:53 2021 -0400

    Internals: Fix some pylint warnings

commit 8814111
Author: Geza Lore <gezalore@gmail.com>
Date:   Fri May 21 14:34:27 2021 +0100

    Rework Ast hashing to be stable (verilator#2974)

    Rework Ast hashing to be stable

    Eliminated reliance on pointer values in AstNode hashes in order to make
    them stable. This requires moving the sameHash functions into a visitor,
    as nodes pointed to via members (and not child nodes) need to be hashed
    themselves.

    The hashes are now stable (as far as I could test them), and the impact
    on verilation time is small enough not to be reliably measurable.

commit fd35492
Author: Geza Lore <gezalore@gmail.com>
Date:   Fri May 21 01:41:46 2021 +0100

    Split V3Hashed to V3Hasher and V3DupFinder (verilator#2967)

    V3Hasher is responsible for computing AstNode hashes, while V3DupFinder
    can be used to find duplicate trees based on hashes. Interface of
    V3DupFinder simplified somewhat. No functional change intended at this
    point, but hash computation might differ in minor details, this however
    should have no perceivable effect on output/runtime.

    Implements (verilator#2964)

commit a44d2b2
Author: Geza Lore <gezalore@gmail.com>
Date:   Thu May 20 11:30:36 2021 +0100

    Move unreleased changes in right place in Changelog

commit aba3883
Author: Wilson Snyder <wsnyder@wsnyder.org>
Date:   Wed May 19 08:14:14 2021 -0400

    Commentary on MULTIDRIVEN (verilator#2972).

commit 9699192
Author: Geza Lore <gezalore@gmail.com>
Date:   Tue May 18 19:28:48 2021 +0100

    Don't merge bit select assignments in C code (verilator#2971)

commit 0f7ec6c
Author: Wilson Snyder <wsnyder@wsnyder.org>
Date:   Mon May 17 18:24:18 2021 -0400

    Fix missing array include (verilator#2966)

commit 471f14d
Author: Geza Lore <gezalore@gmail.com>
Date:   Mon May 17 13:26:24 2021 +0100

    Clean up V3Combine (verilator#2965)

    - Remove dead code
    - Simplify what is left
    - Minor improvements using C++11
    - Remove special case AstNode constructors used only in one place in
      V3Combine (inlined instead).

commit bdc162d
Author: Wilson Snyder <wsnyder@wsnyder.org>
Date:   Sun May 16 18:38:43 2021 -0400

    Fix 16.04 gcc warning

commit 706842d
Author: Wilson Snyder <wsnyder@wsnyder.org>
Date:   Sun May 16 15:11:25 2021 -0400

    CI: Disable Ubuntu-18.04 clang (verilator#2963)

commit 31779b8
Author: Yutetsu TAKATSUKASA <y.takatsukasa@gmail.com>
Date:   Sun May 16 19:01:03 2021 +0900

    Format time string using integer (verilator#2940)

    Co-authored-by: Wilson Snyder <wsnyder@wsnyder.org>

commit 80d62ad
Author: Geza Lore <gezalore@gmail.com>
Date:   Sat May 15 21:10:16 2021 +0100

    Make all AstNode* base class constructors protected. (verilator#2962)

    No functional changes intended. Boring patch in the hope of being
    helpful to new contributors. Although AstNode* base classes needing to
    be abstract is described in the internals manual, this might provide a
    bit of extra safety.

commit 38cab56
Author: Geza Lore <gezalore@gmail.com>
Date:   Sat May 15 18:04:40 2021 +0100

    Add --reloop-limit argument (verilator#2960)

    Add --reloop-limit argument

commit 828f78e
Author: Geza Lore <gezalore@gmail.com>
Date:   Sat May 15 16:05:24 2021 +0100

    Don't emit empty files with low split limits (verilator#2961)

    Attempt to split output files when we know a function will actually be
    emitted, otherwise we might end up with empty files.

commit 5e95cc9
Author: Geza Lore <gezalore@gmail.com>
Date:   Sat May 15 15:56:28 2021 +0100

    Internals: Make AstAlwaysPost an AstNodeProcedure. (verilator#2959)

    This seems to belong there, eliminates some code duplication in V3Clock,
    and also enables splitting AstAlwaysPost statements into different
    functions in V3Order, but should otherwise have little effect.

commit 88fed4b
Author: Wilson Snyder <wsnyder@wsnyder.org>
Date:   Thu May 13 18:56:07 2021 -0400

    Commentary on traces (verilator#2925)

commit 3718fe1
Author: Wilson Snyder <wsnyder@wsnyder.org>
Date:   Thu May 13 18:34:20 2021 -0400

    Commentary (trigger rebuild)

commit a4ab3e1
Author: Ameya Vikram Singh <ameya.v.singh@gmail.com>
Date:   Thu May 13 23:56:53 2021 +0530

    Update latest C++ Standard Compilation flag (verilator#2951)

    For SystemC Project sets the CXX_STANDARD flag from SystemC CMake build config.

commit 9c85426
Author: Yutetsu TAKATSUKASA <y.takatsukasa@gmail.com>
Date:   Thu May 13 06:45:56 2021 +0900

    Internals: Fix build failure on older gcc such as 4.8.5 on CentOS7. No functional change is intended. (verilator#2954)

commit e3b20a3
Author: Yutetsu TAKATSUKASA <y.takatsukasa@gmail.com>
Date:   Wed May 12 00:45:43 2021 +0900

    Internals: Mark VerilatedContextImp::timeFormatSuffix() const (verilator#2947)

commit f7c23c4
Author: Yutetsu TAKATSUKASA <y.takatsukasa@gmail.com>
Date:   Tue May 11 22:38:13 2021 +0900

    Internals: Set missing timescale though it is not referred yet. (verilator#2946)

commit 00cedf3
Author: Yutetsu TAKATSUKASA <y.takatsukasa@gmail.com>
Date:   Tue May 11 22:27:31 2021 +0900

    Tests: Add a test to check if there is overflow or rounding (verilator#2945)

commit 1422c23
Author: Geza Lore <gezalore@gmail.com>
Date:   Tue May 11 12:44:07 2021 +0100

    Split procedures to better respect --output-split-cfuncs (verilator#2942)

    CFuncs only used to be split at procedure (always/initial/final block),
    which on occasion can still yield huge output files if they have large
    procedures. This patch make CFuncs split at statement boundaries within
    procedures. This has the potential to help a lot, but still does not
    help if there are huge statements within procedures.

commit 1a63782
Author: Geza Lore <gezalore@gmail.com>
Date:   Mon May 10 22:15:12 2021 +0100

    Emit 'else if' without nesting. No functional change intended. (verilator#2944)

    Instead of:

    if (a) {
      x;
    } else {
      if (b) {
        y;
      } else {
        if (c) {
          z;
        } else {
          w;
        }
      }
    }

    Emit:

    if (a) {
      x;
    } else if (b) {
      y;
    } else if (c) {
      z;
    } else {
      w;
    }

commit 267c6f6
Author: Geza Lore <gezalore@gmail.com>
Date:   Mon May 10 18:01:11 2021 +0100

    Improve Reloop to accept constant index offsets. (verilator#2939)

    V3Reloop now can roll up indexed assignments between arrays if there is a
    constant offset between indices on the left and right hand sides, e.g.:

    a[0] = b[2];
    a[1] = b[3];
    ...
    a[x] = b[x + 2];

commit 45fbd98
Author: Yutetsu TAKATSUKASA <y.takatsukasa@gmail.com>
Date:   Sun May 9 07:04:22 2021 +0900

    Use V3OptionParser in verilator_coverage (verilator#2935)

    * Internals: Mark unused to avoid compilation failure. No functional change is intended.

    * Use V3OptionParser in VlcMain.cpp

commit f6c0108
Author: Geza Lore <gezalore@gmail.com>
Date:   Sat May 8 20:04:56 2021 +0100

    Optimize large lookup tables to static data (verilator#2926)

    Implements verilator#2925

commit 37d68d3
Author: Jonathan Drolet <jonathan.drolet@riedel.net>
Date:   Sat May 8 08:42:00 2021 -0400

    Support --trace-fst for SystemC with CMake (verilator#2927)

commit 5e56f4d
Author: Yutetsu TAKATSUKASA <y.takatsukasa@gmail.com>
Date:   Sat May 8 21:20:26 2021 +0900

    Tests: Enable undefined behavior sanitizor when --sanitize is set. (verilator#2923)

commit 2bf248b
Author: Jonathan Drolet <jonathan.drolet@gmail.com>
Date:   Sat May 8 08:18:08 2021 -0400

    Add TRACE_THREADS to CMake (verilator#2934)

commit 39ce2f7
Author: Yutetsu TAKATSUKASA <y.takatsukasa@gmail.com>
Date:   Sat May 8 09:45:58 2021 +0900

    Internals: Remove VerilatedVcd::m_suffixesp. No functional change is intended. (verilator#2933)

    Reasons are:
    - it's error prone to keep updating whennever m_suffixes is resized
    - invalid pointer may be set when there is not signal to trace as in t_trace_dumporder_bad

commit 9797af0
Author: Yutetsu TAKATSUKASA <y.takatsukasa@gmail.com>
Date:   Sat May 8 07:16:40 2021 +0900

    Introduce a macro VL_ATTR_NO_SANITIZE_ALIGN to suppress unaligned access check in ubsan (verilator#2929)

    * Add VL_ATTR_NO_SANITIZE_ALIGN macro to disable alignment check of ubsan

    * Mark a function VL_ATTR_NO_SANITIZE_ALIGN because the function is intentionally using unaligned access for the sake of performance.

    Co-authored-by: Wilson Snyder <wsnyder@wsnyder.org>

commit 1e4839e
Author: Yutetsu TAKATSUKASA <y.takatsukasa@gmail.com>
Date:   Sat May 8 01:00:36 2021 +0900

    Fix casting to integer not to cause integer overflow. (verilator#2930)

commit b2139f6
Author: Todd Strader <todd.strader@gmail.com>
Date:   Fri May 7 07:17:54 2021 -0400

    VPI memory access for packed arrays (verilator#2922)

commit 44fd205
Author: Philipp Wagner <mail@philipp-wagner.com>
Date:   Thu May 6 17:07:15 2021 +0100

    Fix make support for gmake 3.x (verilator#2920) (verilator#2921)

    The "file" make function is only available in gmake 4.x, which isn't
    available on RHEL/CentOS 7. Use alternative syntax to support older
    gmake versions.

    Fixes verilator#2920
pieter3d pushed a commit to pieter3d/verilator that referenced this issue Aug 10, 2021
@wsnyder wsnyder added area: wrong runtime result Issue involves an incorrect runtine result from Verilated model and removed area: runtime result labels Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: wrong runtime result Issue involves an incorrect runtine result from Verilated model resolution: fixed Closed; fixed
Projects
None yet
Development

No branches or pull requests

3 participants