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

[Bug]: long dependency paths can break compiler driver #26261

Open
arezaii opened this issue Nov 19, 2024 · 7 comments · May be fixed by #26357
Open

[Bug]: long dependency paths can break compiler driver #26261

arezaii opened this issue Nov 19, 2024 · 7 comments · May be fixed by #26357

Comments

@arezaii
Copy link
Contributor

arezaii commented Nov 19, 2024

Summary of Problem

If chapel dependencies have very long paths, the compiler driver can break.

Description:
This seems to happen mostly with spack dependencies since spack encodes shasums in the package path. The most likely culprits are the CHPL_TARGET_BUNDLED_LINK_ARGS or CHPL_TARGET_BUNDLED_COMPILE_ARGS, but any of the flags that accumulate values longer than 4096 chars can cause this problem.

Is this issue currently blocking your progress?
No, can be worked around by passing --no-compiler-driver to chpl

Steps to Reproduce

  1. Install chapel dependencies such that the resulting CHPL_TARGET_BUNDLED_COMPILE_ARGS or CHPL_TARGET_BUNDLED_LINK_ARGS as viewed from printchplenv --internal is longer than 4096 chars.
  2. Build chapel and try to compile any program.
  3. The assertion at compiler/util/files.cpp:209, assert(strBuf[len - 1] == '\n' && "stored line exceeds maximum length"); will fail.

Source Code:

writeln("Hello");

Compile command:
chpl foo.chpl

Execution command:
n/a, compilation fails

@mstrout
Copy link
Collaborator

mstrout commented Nov 20, 2024

This relates to using Chapel with E4S. I was testing out using Chapel with E4S on my Mac (I recommend homebrew if using Chapel on a Mac is your main goal).

  1. In Docker desktop, Settings, Resources, make sure the virtual disk is > 120GB.
  2. docker run -it --rm ecpe4s/e4s-cuda:24.11-cuda90
  3. spack find -x // to see chapel is part of the available spack packages
  4. spack load chapel
  5. create a hello.chpl with writeln("Hello world!"); in it
  6. chpl --no-compiler-driver hello.chpl // if I don't use the flag, then I get the error Ahmad shows above
  7. ./hello

@riftEmber
Copy link
Member

This happens when the second driver phase is reading a string from a file saved by the first driver phase, for inter-phase communication. The max length of string it will read is hardcoded above at 4096. There's no good reason I know of for this value; I recall picking it because I saw a similar hardcoded limit elsewhere in the compiler for things like path lengths, but I can't find such a limit now so I don't know where I got that from.

Potential solutions Ahmad and I discussed:

  1. Just increase the hardcoded limit (by how much?)
  2. Have the first driver phase keep track of the maximum string length it writes out to a file, and then have it write a file containing that value. Then in the second phase, read that file in first, and use that value as the size of the string buffer.

(1) is simpler but hardcoding still doesn't feel great, and using too large a value means wasting (a small amount of) memory. (2) would be fully future-proof but it also feels bad to add something that complex to accomplish this.

@jabraham17
Copy link
Member

jabraham17 commented Nov 21, 2024

Can this just be solved by reading into a more dynamic buffer like std::string?

std::string readLine(FILE* file) {
  std::string line;
  char buffer[4096];
  while(fgets(buffer, sizeof(buffer), file)) {
    line += buffer;
    // if line ends in newline, trim it and break
    // if eof, break
  }
  return line;
}

This isn't the most optimal, but it avoids hardcoding a max buffer size and is simple

@riftEmber
Copy link
Member

Can this just be solved by reading into a more dynamic buffer like std::string?

Yeah we could, and that's probably the best solution. I had ruled it out a while ago since I thought reading an unlimited amount from a file pointer isn't a good practice, but that might be unfounded. As long as we still don't enable buffer overflows, I think the worst that could happen would be looping for a long time and eventually hiting OOM rather than erroring immediately.

@riftEmber
Copy link
Member

See related #8757 -- I think FILENAME_MAX is what I saw that made me think 4096 was reasonable, as apparently that's a plausible value for it or related constants (https://serverfault.com/questions/9546/filename-length-limits-on-linux). In any case we should move away from it.

@riftEmber
Copy link
Member

PR'd #26310 to fix this

@riftEmber
Copy link
Member

Unfortunately while testing the fix, I found that it works for the driver string length limit, but we then immediately run into uses of FILENAME_MAX that prevent using the restored strings.

For instance, on my Mac I ran the driver in lldb, and between phases changed the main module path (stored in a file) to an 8000 character path. It got past restoring the string, but quickly hit this assert, where the size of executableFilename is FILENAME_MAX + 1. On my Mac I wrote a test program to find FILENAME_MAX is 1024, and on chapdl it's 4096.

So the driver's was just the first length limit encountered, but to actually function with really long paths we'll need to fix all instances of hardcoded length limits (#8757). Will go ahead and do that

@riftEmber riftEmber linked a pull request Dec 4, 2024 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants