close
Skip to content

Added file compatibility check to ELFDynObjParser::parseFile function#1136

Open
Steven6798 wants to merge 1 commit intoqualcomm:mainfrom
Steven6798:issue-1097
Open

Added file compatibility check to ELFDynObjParser::parseFile function#1136
Steven6798 wants to merge 1 commit intoqualcomm:mainfrom
Steven6798:issue-1097

Conversation

@Steven6798
Copy link
Copy Markdown

@Steven6798 Steven6798 commented May 4, 2026

Fixes the ELFDynObjParser::parseFile function. The function
was missing a check that throws an error when incompatible files
are detected.
Update WrongArchInput test to include dynamic linking.
Convert WrongArchInput test into a single common standalone test.
Update linker_faq doc to include information about linking
incompatible files.

Fixes #1097

@quic-shaksuma quic-shaksuma self-requested a review May 4, 2026 17:57
@Steven6798 Steven6798 marked this pull request as ready for review May 4, 2026 18:01
@Steven6798 Steven6798 self-assigned this May 4, 2026
@Steven6798 Steven6798 added this to the 2026/Q02 milestone May 4, 2026
Comment thread test/AArch64/standalone/WrongArchObject/WrongArchInput.test Outdated
Comment thread lib/Readers/ELFDynObjParser.cpp
@Steven6798 Steven6798 marked this pull request as draft May 5, 2026 13:16
Copy link
Copy Markdown
Contributor

@parth-07 parth-07 left a comment

Choose a reason for hiding this comment

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

Do we need to check-in the shared libraries lib2.so in the codebase? Can we instead build the shared libraries as part of the test?

Comment thread test/Common/standalone/WrongArchObject/Inputs/1.yaml
@Steven6798
Copy link
Copy Markdown
Author

Do we need to check-in the shared libraries lib2.so in the codebase? Can we instead build the shared libraries as part of the test?

The shared library would have to be compiled for another target. I don't think it is a good idea to build them since the test would need that additional target to run. I could look into how does the yaml code works to build the lib2.so object file without having to include the binary or require the additional target. Let me know what approach you think is best.

Copy link
Copy Markdown
Contributor

@quic-areg quic-areg left a comment

Choose a reason for hiding this comment

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

I don't think any .so files need to be checked in with this patch. You should be able to produce an arch mismatch by using YAML and EM_NONE:

$ cat 1.yaml
--- !ELF
FileHeader:
  Class:           ELFCLASS32
  Data:            ELFDATA2LSB
  Type:            [[TYPE=ET_REL]]     
  Machine:         EM_NONE              
  ...

$ yaml2obj 1.yaml                  -o tmp.o   
$ yaml2obj 1.yaml -DTYPE=ET_DYN    -o tmp.so 
$ ld.eld -m elf_x86_64 -Bdynamic 1-x64.o ./tmp.so -o out
Fatal: cannot recognize the format of file `./tmp.so'. ...

Then, I think that this could be all one test under Common using %emulation and the target-specific ones (the new ones added in this PR and the existing ones under AArch64/ARM/Hexagon/{linux,standalone}/WrongArchObject) can be deleted.

Comment thread lib/Readers/ELFDynObjParser.cpp
@Steven6798
Copy link
Copy Markdown
Author

I don't think any .so files need to be checked in with this patch. You should be able to produce an arch mismatch by using YAML and EM_NONE:

$ cat 1.yaml
--- !ELF
FileHeader:
  Class:           ELFCLASS32
  Data:            ELFDATA2LSB
  Type:            [[TYPE=ET_REL]]     
  Machine:         EM_NONE              
  ...

$ yaml2obj 1.yaml                  -o tmp.o   
$ yaml2obj 1.yaml -DTYPE=ET_DYN    -o tmp.so 
$ ld.eld -m elf_x86_64 -Bdynamic 1-x64.o ./tmp.so -o out
Fatal: cannot recognize the format of file `./tmp.so'. ...

Then, I think that this could be all one test under Common using %emulation and the target-specific ones (the new ones added in this PR and the existing ones under AArch64/ARM/Hexagon/{linux,standalone}/WrongArchObject) can be deleted.

That's a great idea! I'll add this change to the next commit I have planned for this PR.

@parth-07
Copy link
Copy Markdown
Contributor

parth-07 commented May 7, 2026

@Steven6798 @quic-areg Would it be better to simply have a very simple file with contents int foo() { return 1; } and then use it to build shared library instead of using yaml2obj? Is there any benefit of using yaml2obj here? My concern is that yaml2obj unnecessarily requires much more code than a basic C file.

@Steven6798
Copy link
Copy Markdown
Author

@Steven6798 @quic-areg Would it be better to simply have a very simple file with contents int foo() { return 1; } and then use it to build shared library instead of using yaml2obj? Is there any benefit of using yaml2obj here? My concern is that yaml2obj unnecessarily requires much more code than a basic C file.

Using the simple C file requires another target to be built to create the shared library. I'm not sure if there are other tests that work that way, but I don't think this is a good idea to require multiple targets to be built to run a test unless it is absolutely necessary.

@parth-07
Copy link
Copy Markdown
Contributor

parth-07 commented May 7, 2026

Using the simple C file requires another target to be built to create the shared library. I'm not sure if there are other tests that work that way, but I don't think this is a good idea to require multiple targets to be built to run a test unless it is absolutely necessary.

Okay, I see. You are right. Let's proceed with yaml2obj.

@Steven6798 Steven6798 marked this pull request as ready for review May 7, 2026 14:35
@Steven6798 Steven6798 requested review from parth-07 and quic-areg May 7, 2026 14:35
@quic-seaswara
Copy link
Copy Markdown
Contributor

@Steven6798 can you please squash the commits, and add a good commit message with a description of the change.

Also update the userguide if there is a way to document this behavior.

@Steven6798 Steven6798 force-pushed the issue-1097 branch 2 times, most recently from b88f984 to 15974c2 Compare May 7, 2026 20:36
Fixes the ELFDynObjParser::parseFile function. The function
was missing a check that throws an error when incompatible files
are detected.
Update WrongArchInput test to include dynamic linking.
Convert WrongArchInput test into a single common standalone test.
Update linker_faq doc to include information about linking
incompatible files.

Fixes qualcomm#1097

Signed-off-by: Steven Ramirez Rosa <ramirezr@qti.qualcomm.com>
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

Successfully merging this pull request may close these issues.

linker crashes when a wrong arch shared library is passed in the link line

5 participants