Correct sysroot-prepend conditions for script inputs#809
Correct sysroot-prepend conditions for script inputs#809parth-07 wants to merge 1 commit intoqualcomm:mainfrom
Conversation
quic-seaswara
left a comment
There was a problem hiding this comment.
Please update docs as well.
Please look into this change if verbose logging would be useful.
Map file diagnostics could be a follow up.
| Input *scriptInput = ParentScriptFile->getInput(); | ||
|
|
||
| std::string scriptPath = scriptInput->getResolvedPath().getFullPath(); | ||
| std::string sysrootPath = searchDirs.sysroot().getFullPath(); |
There was a problem hiding this comment.
Will this work with reproduce option ?
There was a problem hiding this comment.
Yes, I think so. We already had this code. Previously, we used to unconditionally prepend the sysroot for script inputs that began with /. Now, we only prepend sysroot for script inputs that begins with / if the parent script is within the sysroot directory.
There was a problem hiding this comment.
Can you please check ?
There was a problem hiding this comment.
I just checked. The linker crashes :'). However, this is not a regression with this patch. We crash without this patch as well.
Reproducer (nightly linker crashes with this):
#!/usr/bin/env bash
mkdir -p A/lib64
mkdir -p A/script
cat >1.c <<\EOF
int foo() { return 1; }
EOF
cat >script.t <<\EOF
GROUP(/lib64/lib1.so)
EOF
cat >main.c <<\EOF
int main() { return 0; }
EOF
clang -o lib1.so -target x86_64-unknown-elf 1.c -shared -fPIC
clang -o main.o -target x86_64-unknown-elf main.c -c
mv lib1.so A/lib64/lib1.so
mv script.t A/script.t
ld.eld --sysroot=A -T A/script.t main.o --reproduce a.tar
rm -rf unpacked && mkdir unpacked
tar -xvf a.tar -C unpacked --strip-components=1
cd unpacked
bash -x response.txtI believe this should be treated as a separate issue.
There was a problem hiding this comment.
I have created an issue to track this #818
| return false; | ||
| if (Type != Input::Script) | ||
| return false; | ||
| if (FileName.empty() || FileName[0] != '/') |
There was a problem hiding this comment.
I am not sure how this will be effective on windows. Needs to be tested on windows too.
| break; | ||
| } | ||
| case InputToken::NameSpec: { | ||
| ThisBuilder.createDeferredInput(Token->name(), Input::Namespec); |
There was a problem hiding this comment.
Can we add the parent script file in all cases, and check if the command is INPUT or GROUP ?
There was a problem hiding this comment.
Can you please explain this in more detail? Currently, we only add parent script for INPUT and GROUP inputs. If I understand correctly, then for the other cases, we do not have any associated parent script.
There was a problem hiding this comment.
I see. Do we need to add this for InputToken::NameSpec too ?
There was a problem hiding this comment.
No, we do not need this for NameSpec. But we do need to fix our behavior for NameSpec with sysroot. GNU LD finds the NameSpec library in all the standard paths inside sysroot. (Paths such as /usr/lib, /lib, /lib64, and so on).
There was a problem hiding this comment.
Please open a follow up ticket on this, We do have this code in eld::ELDEmulateELF but that code is not getting triggered.
There was a problem hiding this comment.
I have created an issue to track this: #819
Is it okay if I push docs as follow-up? I am looking into a framework that will allow us to add extra information to https://qualcomm.github.io/eld/documentation/options/options.html. Currently, this page is automatically generated by parsing the tablegen option files.
Currently, we do have logs of the type |
d3d39f0 to
44c8f15
Compare
44c8f15 to
9bbbb6f
Compare
Yes, this can be a follow up, but the document needs more than autogen treatment to explain the INPUT/GROUP commands appropriately. |
9bbbb6f to
711fb61
Compare
| std::string scriptPath = scriptInput->getResolvedPath().getFullPath(); | ||
| std::string sysrootPath = searchDirs.sysroot().getFullPath(); | ||
|
|
||
| return scriptPath.size() >= sysrootPath.size() && |
There was a problem hiding this comment.
what about a hypothetical /sdk/sysroot and a sibling path /sdk/sysroot_extra? we should probably use llvm::sys::fs::equivalent or similar instead of a raw prefix check
There was a problem hiding this comment.
Here we just need to determine whether the script is within or outside the sysroot. If a script is contained within /sdk/sysroot_extra and the sysroot is /sdk/sysroot, then as per the linker the script is correctly outside the sysroot. The behavior seems correct to me. Can you please let me know if I have missed something?
This commit fixes the logic that determines when the script input path
should be prepended with sysroot. The summary of the new behavior:
- sysroot is only prepended to the script inputs when all the below
constraints are true:
- the input is specified in INPUT/GROUP linker script command.
- The input path begins with '/'.
- The linker script that contains this INPUT/GROUP command is
within the sysroot directory.
- In all other cases, sysroot is not prepended to the script inputs.
Resolves qualcomm#786
Resolves qualcomm#808
Signed-off-by: Parth Arora <partaror@qti.qualcomm.com>
711fb61 to
ef8ca71
Compare
This commit fixes the logic that determines when the script input path should be prepended with sysroot. The summary of the new behavior:
Resolves #786
Resolves #808