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

-s y doesn't work #454

Closed
yuqian-liu opened this issue Nov 30, 2020 · 18 comments
Closed

-s y doesn't work #454

yuqian-liu opened this issue Nov 30, 2020 · 18 comments

Comments

@yuqian-liu
Copy link

-s y option doesn't work. How can we fix this?

./dcm2niix -s y test.DCM
Chris Rorden's dcm2niiX version v1.0.20201102 (JP2:OpenJPEG) (JP-LS:CharLS) GCC5.5.0 x86-64 (64-bit Linux)
Segmentation fault (core dumped)

Thank you

@neurolabusc
Copy link
Collaborator

I believe this is fixed in the development branch. You can test this by building your own copy:

git clone --branch development https://github.com/rordenlab/dcm2niix.git
cd dcm2niix/console
make
./dcm2niix

@yuqian-liu
Copy link
Author

Thank you for reply. This one still doesn't work.

@neurolabusc
Copy link
Collaborator

Fixed with latest commit to development branch. Please test and close this issue once you confirm this resolves your issue.

@yuqian-liu
Copy link
Author

Still have the same issue.
Our command options are:
./dcm2niix -f test.01 -o /tmp/NIFTI -s y /tmp/DICOMM/test.DCM

Thank you

@baxpr
Copy link

baxpr commented Dec 2, 2020

I wonder if this is something about the specific dicom file. It worked ok for me with a different dicom. @yuqian-liu let's check a few different dicom files.

@baxpr
Copy link

baxpr commented Dec 2, 2020

... Ok, we checked a few and the seg fault is still happening with commit 25e314a when the -s y option is used.

@neurolabusc
Copy link
Collaborator

@yuqian-liu I can not replicate. Can you share an example image (send GoogleDrive/DropBox link to email listed in my avatar).

@baxpr
Copy link

baxpr commented Dec 2, 2020

@yuqian-liu @neurolabusc I have it handy, I will send

@baxpr
Copy link

baxpr commented Dec 2, 2020

uname for the server we are testing on:

Linux 3.10.0-1160.2.2.el7.x86_64 #1 SMP Tue Oct 20 16:53:08 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

neurolabusc added a commit that referenced this issue Dec 2, 2020
@neurolabusc
Copy link
Collaborator

Curious, the code leaked when compiled with gcc, but not with Clang. The latest commit should resolve this. By the way, the address sanitizer is a nice way to detect leaks with gcc or clang. So once I had a compiler that elicited the issue, I could run the following to detect the problem:

g++ -O1 -g -fsanitize=address -fno-omit-frame-pointer  -I.  main_console.cpp nii_foreign.cpp nii_dicom.cpp jpg_0XC3.cpp ujpeg.cpp nifti1_io_core.cpp nii_ortho.cpp nii_dicom_batch.cpp  -o dcm2niix -DmyDisableOpenJPEG
./dcm2niix -f test.01 -o /tmp/NIFTI -s y /tmp/DICOMM/test.DCM

@yuqian-liu
Copy link
Author

Thank you The address sanitizer works.

When I run it I got:
usr/bin/ld: cannot find /usr/lib64/libasan.so.0.0.0
collect2: error: ld returned 1 exit status

Then I installed the module and make.

@yuqian-liu
Copy link
Author

Btw, After installed the libasan.so.0.0.0 module, the makefile still won't generate a correct binary. I will have to use the command you provided to get the correct one.

@baxpr
Copy link

baxpr commented Dec 2, 2020

Confirmed - still getting the seg fault with 8725ec5.

@neurolabusc neurolabusc reopened this Dec 2, 2020
@neurolabusc
Copy link
Collaborator

Difficult for me to provide advice, as both Clang and GCC produce no errors on my Mac and Linux systems with the images you provided. I note you are running an Linux kernel from June 2013. Have you ensured your compiler is up to date? Here was my Linux system:

Linux ryzen 5.4.0-56-generic #62-Ubuntu SMP Mon Nov 23 19:20:19 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

@baxpr
Copy link

baxpr commented Dec 2, 2020

Ok, thanks. Given this works:

g++ -O1 -g -fsanitize=address -fno-omit-frame-pointer

And this does not:

g++ -s -O3

Does that give you any hints? Anyway we do have a solution, @yuqian-liu has verified the first one gives a working binary.

...and sounds like it's time to look at our OS version too

@neurolabusc
Copy link
Collaborator

neurolabusc commented Dec 2, 2020

It looks like you are using gcc 5.5.0. I tested gcc 9.3.0 and Clang 12.0. You can always build different optimization levels (-O0= none, -O3 is faster). In theory, different optimization levels should all work the same, but once upon a time the higher levels were known to be a bit buggy (for this reason tools like AFNI only use -O2 to this day). I have not seen any issues myself in the last few years, but you are using a legacy compiler. The only impact of -s should only be to strip the symbol table and relocation information from the executable, which should only decrease the size of the executable. It sounds like your version of gcc has an issue with higher optimization levels. You could try increasing the optimization level until the software crashes, and then run the sanitizer to see if it gives you any insights, e.g.

g++ -O3 -g -fsanitize=address -foo-omit-frame-pointer

Alternatively, just run the -O1 version.

@baxpr
Copy link

baxpr commented Dec 2, 2020

Thanks much! I reckon we're properly sorted so @yuqian-liu can you close this again?

@baxpr
Copy link

baxpr commented Dec 23, 2020

@neurolabusc I am no longer able to replicate the problem with commit 25e314a. Looks like we had a system update:

Linux theta.vuiis.org 3.10.0-1160.11.1.el7.x86_64 #1 SMP Fri Dec 18 16:34:56 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

The new version (development branch) works fine too, for what that's worth.

yarikoptic added a commit to neurodebian/dcm2niix that referenced this issue Apr 6, 2021
* commit 'v1.0.20200427-97-g0587941': (22 commits)
  Overflow for Siemens data [missing protocol name] (rordenlab#466)
  MacOS notarization, minor tweaks
  Increase details for series stacking, enhance merge override mode (rordenlab#463)
  Bump version date
  Retain Philips Scaling Values for images where scaling does not vary but volumes must be separated (rordenlab#461)
  Support huge PAR/REC files (rordenlab#460)
  Prevent dti4D overflow
  Fix PAR/REC ADC detection (rordenlab#462)
  DICOM field map calibrated in Hz given name _fieldmaphz (rordenlab#455)
  PAR/REC field map calibrated in Hz given name _fieldmaphz (rordenlab#455)
  Ignore LocationsInAcquisition (0020,1002) if number of slices is not evenly divisible with this value (rordenlab#459)
  Convert DICOM series where bit allocated (0028,0100) varies across slices (rordenlab#458)
  Clear RepetitionTimeExcitation (rordenlab#457)
  leak (rordenlab#454)
  Single file mode memory allocation (rordenlab#454)
  When -n is specified, only save BIDS for requested series (rordenlab#453)
  Use 1st study time if multiple times provided.
  Apple. M1. use C++ not. C
  Fix CMake for Apple Silicon (note similar change required for CloudFlare zlib)
  Only report b-value for GE data with 0043,1039 if EPI (0018,9018) (rordenlab#449)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants