Page MenuHomePhabricator

[flang] GET_COMMAND_ARGUMENT(LENGTH) runtime implementation
ClosedPublic

Authored by rovka on Sep 3 2021, 4:48 AM.

Details

Summary

Implement the ArgumentLength entry point of GET_COMMAND_ARGUMENT. Also
introduce a fixture for the tests.

Note that this also changes the interface for ArgumentLength from
returning a 4-byte integer to returning an 8-byte integer.

Diff Detail

Event Timeline

rovka created this revision.Sep 3 2021, 4:48 AM
rovka requested review of this revision.Sep 3 2021, 4:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2021, 4:48 AM
klausler added inline comments.Sep 3 2021, 10:07 AM
flang/runtime/command.cpp
23

It'll probably never happen, but what if the length of the argument is > 2GiB?

rovka updated this revision to Diff 370868.Sep 6 2021, 2:31 AM

Made it fail when strlen returns something we can't represent.

klausler added inline comments.Sep 6 2021, 12:13 PM
flang/runtime/command.cpp
14

I don't think that this is the right solution. What if the LENGTH= argument to GET_COMMAND_ARGUMENT had been an INTEGER*8 variable? That case should work. It would be better to use a larger result type here.

rovka added inline comments.Sep 8 2021, 12:55 AM
flang/runtime/command.cpp
14

Oh, I misunderstood, I thought we had agreed on the interface.

I'll upload a new version with 8-byte integers next week, since right now I'm a bit busy with some side issues.

rovka updated this revision to Diff 372179.Sep 13 2021, 1:41 AM
rovka edited the summary of this revision. (Show Details)

Switched from 4-byte to 8-byte integer.

rovka updated this revision to Diff 372652.Sep 15 2021, 1:32 AM

Also add to the no-cpp-dep test (would be nice to have most of the runtime in there).

rovka updated this revision to Diff 373518.Sep 20 2021, 2:09 AM

Rebase and move the definitions for CountType and LengthType to the header file, as suggested in https://reviews.llvm.org/D109813

klausler added inline comments.Sep 27 2021, 10:56 AM
flang/include/flang/Runtime/command.h
19

I think that you can use std::int32_t/std::int64_t here.

rovka updated this revision to Diff 375407.Sep 27 2021, 2:33 PM

Switched to std::int32_t/64_t

klausler added inline comments.Sep 27 2021, 2:45 PM
flang/include/flang/Runtime/command.h
19

I meant that you don't need to have the type aliases CountType, LengthType, &c. either.

31–32

std::int32_t

rovka updated this revision to Diff 375426.Sep 27 2021, 3:47 PM

Remove shorthand types.

klausler accepted this revision.Sep 27 2021, 3:49 PM
This revision is now accepted and ready to land.Sep 27 2021, 3:49 PM
This revision was automatically updated to reflect the committed changes.

I'm seeing build failure with gcc 9.3.0 with this patch.

******************** TEST 'Flang :: Runtime/no-cpp-dep.c' FAILED ********************
Script:
--
: 'RUN: at line 8';   /home/sw/thirdparty/gcc/gcc-9.3.0/linux86-64/bin/gcc -std=c99 llvm-project/flang/test/Runtime/no-cpp-dep.c -Illvm-project/flang/include llvm-project/build/lib/libFortranRuntime.a llvm-project/build/lib/libFortranDecimal.a -lm -o /dev/null
--
Exit Code: 1

Command Output (stderr):
--
In file included from /home/sw/thirdparty/gcc/gcc-9.3.0/linux86-64/lib/gcc/x86_64-pc-linux-gnu/9.3.0/include/stdint.h:9,
                 from llvm-project/flang/test/Runtime/no-cpp-dep.c:12:
/usr/include/stdint.h:26:10: fatal error: bits/libc-header-start.h: No such file or directory
   26 | #include <bits/libc-header-start.h>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

--

********************
********************
Failed Tests (1):
  Flang :: Runtime/no-cpp-dep.c


Testing Time: 2.52s
  Unsupported      :    3
  Passed           : 1012
  Expectedly Failed:    3
  Failed           :    1

Hi Valentin,

Thank you for the report!

I've been googling that error message a bit and all I could find were posts
related to compiling with -m32 on a 64-bit system. However the test is
definitely not passing -m32 and you seem to be using an x86-64 compiler, so
I'm not sure what the issue is. Would it be possible for you to try some
experiments on your end, e.g. check if you have libc-header-start.h
anywhere on your system, or installing gcc-multilib?

Thanks,
Diana

Hi Valentin,

Thank you for the report!

I've been googling that error message a bit and all I could find were posts
related to compiling with -m32 on a 64-bit system. However the test is
definitely not passing -m32 and you seem to be using an x86-64 compiler, so
I'm not sure what the issue is. Would it be possible for you to try some
experiments on your end, e.g. check if you have libc-header-start.h
anywhere on your system, or installing gcc-multilib?

Thanks,
Diana

We have libc-header-start.h on the system under /usr/include/x86_64-linux-gnu/bits/libc-header-start.h. Since it's a lab machine I cannot install gcc-multilib easily.

rovka added a comment.Sep 30 2021, 1:04 AM

Hi Valentin,

I tried to reproduce this in a Ubuntu Focal container with gcc 9.3.0
installed via apt (and no multilib afaict) and I'm not seeing any failures.
It also seems to work in Compiler Explorer: https://godbolt.org/z/nq8czercf
I think the issue might be limited to your system. If you find some kind of
workaround to get the test to work on your system too I'll be happy to
review any changes.

Cheers,
Diana

clementval added a comment.EditedSep 30 2021, 1:24 AM

Hi Valentin,

I tried to reproduce this in a Ubuntu Focal container with gcc 9.3.0
installed via apt (and no multilib afaict) and I'm not seeing any failures.
It also seems to work in Compiler Explorer: https://godbolt.org/z/nq8czercf
I think the issue might be limited to your system. If you find some kind of
workaround to get the test to work on your system too I'll be happy to
review any changes.

Cheers,
Diana

Thanks for trying to reproduce. No worries, I'll check on my side.

Interestingly it doesn't fail if I run the command by hand. Might be a llvm configuration problem