[PATCH] D62838: [Object] add isExecutableObject member function

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:1203
+template <class ELFT> bool ELFObjectFile<ELFT>::isExecutableObject() const {
+  return EF.getHeader()->e_type == ELF::ET_EXEC;
+}
----------------
abrachet wrote:
> MaskRay wrote:
> > abrachet wrote:
> > > MaskRay wrote:
> > > > Some people will be surprised if `ET_DYN` is not considered as executable object.
> > > > 
> > > > Position-Independent Executables use `ET_DYN`.
> > > $ llvm-readelf `which llvm-readelf` -h
> > > ...
> > > Type:                              DYN (Shared object file)
> > > 
> > > Oh yeah, good catch, thank you. How would you write this method? Does an executable always have e_entry as non zero? Is there ever a case where execve(2) will not fail if e_entry is 0? 
> > > 
> > > Also, should I worry about this? https://github.com/freebsd/freebsd/blob/master/sys/kern/imgact_elf.c#L1135 I don't know enough about ELF yet on this one. Thanks.
> > Do you have a use case (not a unittest) of this member function?
> > 
> > If you have one, let's see if relocatable/executable/shared should be trichotomy or not.
> > 
> > Trichotomy doesn't work in ELF. One may argue a PIE doesn't have PT_INTERP but a DSO doesn't. However, a static pie doesn't have PT_INTERP, either. Its difference from a DSO is if DT_DEBUG exists. However, DT_DEBUG can be disabled in lld with -z rodynamic (Fuchsia people do so)...
> > 
> > > Also, should I worry about this? https://github.com/freebsd/freebsd/blob/master/sys/kern/imgact_elf.c#L1135
> > 
> > We should not be bothered by FreeBSD Brandinfo
> > Do you have a use case (not a unittest) of this member function?
> Yes, D62718 here I have a change which tries to mirror gnu objcopy's handling of output file permissions which is dependent on the file being executable or not. 
> 
> Thanks for the help.
Replied on D62718. The idea to use isExecutableObject() to set file permissions looks very bad to me. That would bring more issues than it tried to solve. It is a good example that copying every corner case of GNU objcopy is harmful.

If D62718 is the only use case, I suggest we wait until a more reasonable use case arises.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62838/new/

https://reviews.llvm.org/D62838