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

Mmap: fix grow! for non file IOs #55849

Merged
merged 4 commits into from
Sep 24, 2024

Conversation

IanButterworth
Copy link
Sponsor Member

Fixes #54203
Requires #55641

Based on #55641 (comment)
cc. @JakeZw @ronisbr

@ronisbr
Copy link
Sponsor Member

ronisbr commented Sep 23, 2024

Oh, that was fast :) Thanks @IanButterworth ! LGTM!

@JakeZw can you test in your setup, please, applying the changes in #55641 and here?

@IanButterworth

This comment was marked as resolved.

@IanButterworth
Copy link
Sponsor Member Author

Hitting an error

  | 2024-09-23 21:44:31 EDT | SharedArrays                                     (1) \|         failed at 2024-09-23T18:44:32.107
  | 2024-09-23 21:44:32 EDT | Error During Test at /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-honeycrisp-R17H3W25T9.0/build/default-honeycrisp-R17H3W25T9-0/julialang/julia-master/julia-a7c648dd99/share/julia/test/testdefs.jl:24
  | 2024-09-23 21:44:32 EDT | Got exception outside of a @test
  | 2024-09-23 21:44:32 EDT | LoadError: stat returned zero type for a valid path
  | 2024-09-23 21:44:32 EDT | Stacktrace:
  | 2024-09-23 21:44:32 EDT | [1] error(s::String)
  | 2024-09-23 21:44:32 EDT | @ Base ./error.jl:44
  | 2024-09-23 21:44:32 EDT | [2] stat(fd::RawFD)
  | 2024-09-23 21:44:32 EDT | @ Base.Filesystem ./stat.jl:180
  | 2024-09-23 21:44:32 EDT | [3] stat
  | 2024-09-23 21:44:32 EDT | @ ./iostream.jl:63 [inlined]
  | 2024-09-23 21:44:32 EDT | [4] isfile
  | 2024-09-23 21:44:32 EDT | @ ./stat.jl:503 [inlined]
  | 2024-09-23 21:44:32 EDT | [5] mmap(io::IOStream, ::Type{Array{Int64, 3}}, dims::Tuple{Int64, Int64, Int64}, offset::Int64; grow::Bool, shared::Bool)
  | 2024-09-23 21:44:32 EDT | @ Mmap /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-honeycrisp-R17H3W25T9.0/build/default-honeycrisp-R17H3W25T9-0/julialang/julia-master/julia-a7c648dd99/share/julia/stdlib/v1.12/Mmap/src/Mmap.jl:224
  | 2024-09-23 21:44:32 EDT | [6] _shm_mmap_array(T::Type, dims::Tuple{Int64, Int64, Int64}, shm_seg_name::String, mode::UInt32)
  | 2024-09-23 21:44:32 EDT | @ SharedArrays /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-honeycrisp-R17H3W25T9.0/build/default-honeycrisp-R17H3W25T9-0/julialang/julia-master/julia-a7c648dd99/share/julia/stdlib/v1.12/SharedArrays/src/SharedArrays.jl:692
  | 2024-09-23 21:44:32 EDT | [7] shm_mmap_array(T::Type, dims::Tuple{Int64, Int64, Int64}, shm_seg_name::String, mode::UInt32)
  | 2024-09-23 21:44:32 EDT | @ SharedArrays /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-honeycrisp-R17H3W25T9.0/build/default-honeycrisp-R17H3W25T9-0/julialang/julia-master/julia-a7c648dd99/share/julia/stdlib/v1.12/SharedArrays/src/SharedArrays.jl:651
  | 2024-09-23 21:44:32 EDT | [8] SharedArray{Int64, 3}(dims::Tuple{Int64, Int64, Int64}; init::Function, pids::Vector{Int64})
  | 2024-09-23 21:44:32 EDT | @ SharedArrays /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-honeycrisp-R17H3W25T9.0/build/default-honeycrisp-R17H3W25T9-0/julialang/julia-master/julia-a7c648dd99/share/julia/stdlib/v1.12/SharedArrays/src/SharedArrays.jl:118
  | 2024-09-23 21:44:32 EDT | [9] SharedArray
  | 2024-09-23 21:44:32 EDT | @ /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-honeycrisp-R17H3W25T9.0/build/default-honeycrisp-R17H3W25T9-0/julialang/julia-master/julia-a7c648dd99/share/julia/stdlib/v1.12/SharedArrays/src/SharedArrays.jl:104 [inlined]
  | 2024-09-23 21:44:32 EDT | [10] #shmem_rand#73
  | 2024-09-23 21:44:32 EDT | @ /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-honeycrisp-R17H3W25T9.0/build/default-honeycrisp-R17H3W25T9-0/julialang/julia-master/julia-a7c648dd99/share/julia/stdlib/v1.12/SharedArrays/src/SharedArrays.jl:548 [inlined]
  | 2024-09-23 21:44:32 EDT | [11] shmem_rand(TR::UnitRange{Int64}, dims::Tuple{Int64, Int64, Int64})
  | 2024-09-23 21:44:32 EDT | @ SharedArrays /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-honeycrisp-R17H3W25T9.0/build/default-honeycrisp-R17H3W25T9-0/julialang/julia-master/julia-a7c648dd99/share/julia/stdlib/v1.12/SharedArrays/src/SharedArrays.jl:546

Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you just need to swap the order of the requestedSizeLarger check later, since it it already guards against being Mmap.Anonymous

stdlib/Mmap/src/Mmap.jl Outdated Show resolved Hide resolved
stdlib/Mmap/src/Mmap.jl Outdated Show resolved Hide resolved
@JakeZw
Copy link

JakeZw commented Sep 24, 2024

@ronisbr asked me to test on my PI. I will gladly do this, however my skills figuring out how to test are sorely lacking. Please walk me through the details.

@IanButterworth
Copy link
Sponsor Member Author

Once CI is green here:

  1. Install juliaup
  2. juliaup add pr55849
  3. julia +pr55849
  4. Run test

@JakeZw
Copy link

JakeZw commented Sep 24, 2024

For CI to be green do the 3 green squares and 1 red square at the top right of this page have to be all green? (+5-3)

@ronisbr
Copy link
Sponsor Member

ronisbr commented Sep 24, 2024

@JakeZw My ideia is for testing if you can use BaremetalPi.jl in your Raspberry Pi with those modifications. I think loading BaremetalPi.jl and trying to read an IO is sufficient.

Regarding the CI, I think you need to wait the Building process.

@JakeZw
Copy link

JakeZw commented Sep 24, 2024

I need to know how I can tell that the building process is complete.

@JakeZw
Copy link

JakeZw commented Sep 24, 2024

Maybe if I read just above this comment box it say Building Pending -
STarted, it will tell me when it is complete, Sorry I just need to read!

@JakeZw
Copy link

JakeZw commented Sep 24, 2024

The statements that were erroring. namely

        gpiomem_io  = open("/dev/gpiomem", "w+")
        gpiomem_map = Mmap.mmap(
            gpiomem_io,
            Vector{UInt32},
            (1024,),
            0,
            grow = false
        )

Now work!

My application that depends upon BareMetalPI also works. Thanks to you all

@IanButterworth IanButterworth added backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 labels Sep 24, 2024
@IanButterworth IanButterworth merged commit b0db75d into JuliaLang:master Sep 24, 2024
9 checks passed
@IanButterworth IanButterworth deleted the ib/mmap_grow_rpi branch September 24, 2024 20:42
KristofferC pushed a commit that referenced this pull request Sep 30, 2024
Fixes #54203
Requires #55641

Based on
#55641 (comment)
cc. @JakeZw @ronisbr

---------

Co-authored-by: Jameson Nash <[email protected]>
(cherry picked from commit b0db75d)
@KristofferC KristofferC mentioned this pull request Sep 30, 2024
39 tasks
KristofferC added a commit that referenced this pull request Oct 1, 2024
Backported PRs:
- [x] #55849 <!-- Mmap: fix grow! for non file IOs -->
- [x] #55863 <!-- Update TaskLocalRNG docstring according to #49110 -->
- [x] #54433 <!-- Root globals in toplevel exprs -->
- [x] #55828 <!-- Fix some corner cases of `isapprox` with unsigned
integers -->
- [x] #55890 <!-- Profile: fix order of fields in heapsnapshot & improve
formatting -->
- [x] #55884 <!-- inference: add missing `TypeVar` handling for
`instanceof_tfunc` -->
- [x] #55881 <!-- Install terminfo data under /usr/share/julia -->
- [x] #55909 <!-- do not intentionally suppress errors in precompile
script from being reported or failing the result -->
- [x] #55355 <!-- relocation: account for trailing path separator in
depot paths -->
- [x] #55917 <!-- fix rawbigints OOB issues -->
- [x] #55892 <!-- TOML: Avoid type-pirating `Base.TOML.Parser` -->
- [x] #55798 <!-- Broadcast binary ops involving strided triangular -->
- [x] #55919 <!-- Limit `@inbounds` to indexing in the dual-iterator
branch in `copyto_unaliased!` -->

Contains multiple commits, manual intervention needed:
- [ ] #54009 <!-- allow extensions to trigger from packages in [deps]
-->
- [ ] #55509 <!-- Fix cong implementation to be properly random and not
just cycling. -->
- [ ] #55569 <!-- Add a docs section about loading/precomp/ttfx time
tuning -->
- [ ] #55824 <!-- Replace regex package module checks with actual code
checks -->

Non-merged PRs with backport label:
- [ ] #55932 <!-- REPL: make UndefVarError aware of imported modules -->
- [ ] #55910 <!-- Prevent extensions from blocking parallel
pre-compilation -->
- [ ] #55908 <!-- add logic to prefer loading modules that are already
loaded -->
- [ ] #55886 <!-- irrationals: restrict assume effects annotations to
known types -->
- [ ] #55871 <!-- lowering: don't reverse handler order in
`(pop-handler-list ...)` -->
- [ ] #55870 <!-- fix infinite recursion in `promote_type` for
`Irrational` -->
- [ ] #55867 <!-- update `hash` doc string: `widen` not required any
more -->
- [ ] #55851 <!-- [REPL] Fix #55850 by using `safe_realpath` instead of
`abspath` in `projname` -->
- [ ] #55813 <!-- Check for conflicting `@ccallable` name before JIT
registration -->
- [ ] #55743 <!-- doc: heap snapshot viewing -->
- [ ] #55741 <!-- Change annotations to use a NamedTuple -->
- [ ] #55534 <!-- Set stdlib sources as read-only during installation
-->
- [ ] #55499 <!-- propagate the terminal's `displaysize` to the
`IOContext` used by the REPL -->
- [ ] #55458 <!-- Allow for generically extracting unannotated string
-->
- [ ] #55457 <!-- Make AnnotateChar equality consider annotations -->
- [ ] #55220 <!-- `isfile_casesensitive` fixes on Windows -->
- [ ] #55169 <!-- `propertynames` for SVD respects private argument -->
- [ ] #54457 <!-- Make `String(::Memory)` copy -->
- [ ] #53957 <!-- tweak how filtering is done for what packages should
be precompiled -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
- [ ] #50813 <!-- More doctests for Sockets and capitalization fix -->
- [ ] #50157 <!-- improve docs for `@inbounds` and
`Base.@propagate_inbounds` -->
@KristofferC KristofferC mentioned this pull request Oct 7, 2024
44 tasks
KristofferC pushed a commit that referenced this pull request Oct 7, 2024
Fixes #54203
Requires #55641

Based on
#55641 (comment)
cc. @JakeZw @ronisbr

---------

Co-authored-by: Jameson Nash <[email protected]>
(cherry picked from commit b0db75d)
@KristofferC KristofferC mentioned this pull request Oct 7, 2024
63 tasks
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.10 Change should be backported to the 1.10 release stdlib Julia's standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mmap.mmap regression from Julia 1.10.0 to 1.10.1
5 participants