kristina created this revision.
kristina added reviewers: rnk, echristo, clang, rjmccall.
Herald added a subscriber: cfe-commits.
Attempt to unbreak behavior caused by https://reviews.llvm.org/D44491 resulting
in inability to build standalone/unbridged CoreFoundation on an ELF target with
`-fcon
kristina added a comment.
Does appear to work properly:
cfstring:002AC640 off_2AC640 dq offset
__CFConstantStringClassReference
cfstring:002AC640 ; DATA
XREF: skipDTD+19D↑o
cfstring:002AC640
compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.
This looks fine to me, but this definitely should have an accompanying test.
As to the other items you mention, the current section naming actually is
important as it enables the CFString
rjmccall added inline comments.
Comment at: lib/CodeGen/CodeGenModule.cpp:4108
+ // consistent with old behavior for this target as it would fail
+ // on the cast<> instead.
+ assert(GV && "isa isn't a GlobalValue");
compnerd wrote:
> I think that
kristina added a comment.
Would you be okay with me renaming `cfstring` to `.cfstring` for ELF so it's in
line with ELF section naming conventions (I'm not entirely sure if that could
cause issues with ObjC stuff)? And yes I think it's best to avoid that
code-path altogether if it turns out to
rjmccall added a reviewer: theraven.
rjmccall added a comment.
In https://reviews.llvm.org/D52344#1242451, @kristina wrote:
> Would you be okay with me renaming `cfstring` to `.cfstring` for ELF so it's
> in line with ELF section naming conventions (I'm not entirely sure if that
> could cause i
smeenai added a comment.
In https://reviews.llvm.org/D52344#1242530, @rjmccall wrote:
> In https://reviews.llvm.org/D52344#1242451, @kristina wrote:
>
> > Would you be okay with me renaming `cfstring` to `.cfstring` for ELF so
> > it's in line with ELF section naming conventions (I'm not entirel
kristina updated this revision to Diff 166576.
kristina added a comment.
Does this look about right? I'm not sure what's up with DSO local attribute, it
doesn't seem to apply, I think this test makes sense though. `ninja
check-clang-codegen` succeeded this time:
Testing Time: 12.77s
Expec
kristina marked 2 inline comments as done.
kristina added a comment.
Binary layout also looks sane (compiled with `-fPIC -faddrsig -Wl,--icf=safe`),
just digging it through it in a dissasembler:
[/q/src/clwn]$ llvm-readobj -elf-output-style=GNU -sections
/q/org.llvm.caches/clownschool/clwn-sy
kristina marked 3 inline comments as done.
kristina added a comment.
Want to see what @rnk has to say about this before landing it since he wrote
the original code and if my understanding of `common` vs `dso_local` is
accurate or not since I don't have much experience with the Windows specific
theraven added a comment.
In https://reviews.llvm.org/D52344#1242530, @rjmccall wrote:
> In https://reviews.llvm.org/D52344#1242451, @kristina wrote:
>
> > Would you be okay with me renaming `cfstring` to `.cfstring` for ELF so
> > it's in line with ELF section naming conventions (I'm not entire
kristina added a comment.
In https://reviews.llvm.org/D52344#1243149, @rjmccall wrote:
> I can respect wanting to change the rules on ELF, but it sounds like we do
> need to stick with the current section names. Absent an ABI break to stop
> looking for the existing section names, CF will misb
kristina closed this revision.
kristina added a comment.
Closed by https://reviews.llvm.org/rL342883
(https://reviews.llvm.org/rC342883). Manually closing it as Phabricator is
slightly broken, hopefully it makes the links when it catches up.
Repository:
rC Clang
https://reviews.llvm.org/D52
kristina added a comment.
Alright, I guess I'll land it if there's no objections to it, `cfstring` is
staying as is, the PE/COFF codepath is not affected in terms of functionality
and test is fine I think, hopefully `x86_64-elf` will not yield different
results on different machines in terms of
kristina reopened this revision.
kristina added a comment.
This revision is now accepted and ready to land.
Cascade of build failures stemming from `GV->setSection(".rodata");`, reverted
the commit, it seems that `CFString.c` is causing all those issues despite
passing when ran locally.
Reposi
kristina added a comment.
Turns out test didn't execute as there are two tests named `CFString.c` and
`cfstring.c` which usually shouldn't matter unless LLVM source checkout is done
into a case insensitive filesystem in which case the failing test is skipped,
this seems like a bug, going to ren
kristina updated this revision to Diff 166996.
kristina added a comment.
Split up the tests for ELF into tests 1). Checking whether CF can be built and
linked correctly 2). Asm section tests originally from `CFStrings.c` as they
had additional ELF-related cruft that should have been a separate t
This revision was automatically updated to reflect the committed changes.
Closed by commit rL343038: Reland "[Clang][CodeGen][ObjC]: Fix
CoreFoundation on ELF with `-fconstant… (authored by kristina, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://revie
On Sun, Sep 23, 2018 at 5:39 AM David Chisnall via Phabricator <
revi...@reviews.llvm.org> wrote:
> theraven added a comment.
>
> In https://reviews.llvm.org/D52344#1242530, @rjmccall wrote:
>
> > In https://reviews.llvm.org/D52344#1242451, @kristina wrote:
> >
> > > Would you be okay with me rena
19 matches
Mail list logo
