Author: honggyu.kim
Date: Thu Sep 1 06:29:21 2016
New Revision: 280355
URL: http://llvm.org/viewvc/llvm-project?rev=280355&view=rev
Log:
[Frontend] Fix mcount inlining bug
Since some profiling tools, such as gprof, ftrace, and uftrace, use
-pg option to generate a mcount function call at the ent
This revision was automatically updated to reflect the committed changes.
Closed by commit rL280355: [Frontend] Fix mcount inlining bug (authored by
honggyu.kim).
Changed prior to commit:
https://reviews.llvm.org/D22666?vs=68954&id=69986#toc
Repository:
rL LLVM
https://reviews.llvm.org/D226
Author: honggyu.kim
Date: Thu Sep 1 01:14:45 2016
New Revision: 280337
URL: http://llvm.org/viewvc/llvm-project?rev=280337&view=rev
Log:
Remove whitespace to test commit access
Modified:
cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
URL:
htt
honggyu.kim added a reviewer: compnerd.
honggyu.kim removed a subscriber: compnerd.
honggyu.kim updated this revision to Diff 68954.
honggyu.kim added a comment.
I have fixed SOH character problem by https://reviews.llvm.org/D23792. I've
verified that mcount.c and gnu-mcount.c tests are passed n
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#513465, @compnerd wrote:
> No, the inserted character is a literal SOH, not the string "\01".
I don't know why the string is passed in a different way but '\01' is shown in
IR previously as below
$ clang -target armv7-unknown-n
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#509904, @compnerd wrote:
> Theres a frontend mangler and a backend mangler. You should try this on
> Windows GNU environments :-).
Thanks for the confirmation :)
If so, would it be okay if I put one more backslash for each mcount
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#506884, @compnerd wrote:
> The `\01` is to prevent the mangler from touching the function name. If you
> noticed the check tags in the quoted test, some targets expect it to be
> decorated and others do not. What exactly do you m
honggyu.kim added a comment.
I see the difference now.
Below is the originally generated IR without this patch. It shows `\01`
clearly in `call void @"\01__gnu_mcount_nc"()`.
$ clang -target armv7-unknown-none-eabi -pg -meabi gnu -S -emit-llvm -o -
test-mcount.c
; ModuleID = 'mcount.c'
s
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#506884, @compnerd wrote:
> The `\01` is to prevent the mangler from touching the function name. If you
> noticed the check tags in the quoted test, some targets expect it to be
> decorated and others do not. What exactly do you m
honggyu.kim added a comment.
Hi Renato and Saleem,
I found one more testcase that has to be changed. It's in
`llvm/tools/clang/test/Frontend/gnu-mcount.c`.
But strangely, some cases provide mcount name with prefix '\01' such as below:
class ARMTargetInfo : public TargetInfo {
...
public
honggyu.kim updated the summary for this revision.
honggyu.kim updated this revision to Diff 66276.
honggyu.kim added a comment.
I've updated the testcase again it adds 2 more tests for -O2 and without -pg.
Please have a look at it.
$ llvm-lit mcount.c
-- Testing: 1 tests, 1 threads --
PASS
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#500329, @hfinkel wrote:
> In this case, I think that making a simple test (changing the current test to
> be) like test/CodeGen/stackrealign.c would be fine. If you have any
> questions, please feel free to ask.
Thanks. Please co
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#500327, @rjmccall wrote:
> In https://reviews.llvm.org/D22666#500326, @honggyu.kim wrote:
>
> > Should we also modify clang/test/CodeGen/mcount.c as well? I'm not
> > actually familiar with test infra.
>
>
> Yes, you'll need to mod
honggyu.kim added a comment.
Should we also modify clang/test/CodeGen/mcount.c as well? I'm not actually
familiar with test infra.
$ cat llvm/tools/clang/test/CodeGen/mcount.c
// RUN: %clang_cc1 -pg -triple i386-unknown-unknown -emit-llvm -o - %s |
FileCheck %s
// RUN: %clang_cc1 -pg -tr
honggyu.kim marked 4 inline comments as done.
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#499583, @hfinkel wrote:
> Comments about the comment, but otherwise, LGTM.
Just updated the comment as you mentioned. Thanks for correcting my English :)
https://reviews.llvm.org/D226
honggyu.kim updated the summary for this revision.
honggyu.kim updated this revision to Diff 66072.
https://reviews.llvm.org/D22666
Files:
lib/CodeGen/CodeGenFunction.cpp
Index: lib/CodeGen/CodeGenFunction.cpp
===
--- lib/CodeGen/
honggyu.kim added a comment.
I just wrote how I tested this in the bugzilla report page below:
https://llvm.org/bugs/show_bug.cgi?id=28660
https://reviews.llvm.org/D22666
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/
honggyu.kim updated this revision to Diff 65879.
honggyu.kim added a comment.
I've updated the diff that can be working with https://reviews.llvm.org/D22825.
I appreciate your work in backend, Hal.
Please correct me if I'm wrong.
https://reviews.llvm.org/D22666
Files:
lib/CodeGen/CodeGenFun
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#496355, @hfinkel wrote:
> Here's a patch for the backend part: https://reviews.llvm.org/D22825 - If
> this makes sense to everyone, we can change the frontend to add the
> corresponding attribute instead of inserting the function c
honggyu.kim added a comment.
One more problem is that -finstrument-functions has the same inlining problem.
The code in `llvm/tools/clang/lib/CodeGen/CodeGenFunction.cpp` shows that.
void CodeGenFunction::StartFunction(GlobalDecl GD,
QualType RetTy,
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#493708, @rjmccall wrote:
> Note that the presence of the mcount call itself will significantly impact
> inlining and, really, the entire optimization pipeline. Having this be a
> late pass seems more in keeping with what I assume
honggyu.kim added a comment.
In https://reviews.llvm.org/D22666#493109, @rjmccall wrote:
> In https://reviews.llvm.org/D22666#493026, @hfinkel wrote:
>
> > What's the actual desired behavior here? Should the inliner strip out calls
> > to mcount() as it inlines?
>
>
> I think they basically just
honggyu.kim added a comment.
Hi John and Hans,
I don't know if I found right reviewers for this patch, but could you please
have a look at it?
I found this problem while I was trying to profile a binary that is compiled
with -pg and -O2 as I reported in bugzilla.
https://llvm.org/bugs/show_bug.
honggyu.kim created this revision.
honggyu.kim added reviewers: rjmccall, hans.
honggyu.kim added a subscriber: cfe-commits.
Since some profiling tools, such as gprof, ftrace, and uftrace, use
-pg option to generate a mcount function call at the entry of each
function. Function invocation can be d
honggyu.kim added a comment.
In http://reviews.llvm.org/D12906#274724, @o.gyorgy wrote:
> Hi,
>
> You are right the diff is is based on the hash. We already tried to
> use an earlier hash generator (before the patch was introduced), which
> generates a slightly different plist, that is why the
honggyu.kim added a comment.
In http://reviews.llvm.org/D12906#273089, @xazax.hun wrote:
> By default it is disabled for security reasons. You can enable by passing a
> command line option to the code checker, something like --not-host-only.
Thanks, I can remotely access now with --not-host-on
honggyu.kim added a comment.
In http://reviews.llvm.org/D12906#272257, @dkrupp wrote:
> Hi Anna & Kim,
Hi Daniel,
> we recognized these scalability issues you just described and that's why we
> created CodeChecker https://github.com/Ericsson/codechecker/
> tool. A HTML report viewer for Cla
honggyu.kim added a comment.
Hi Anna,
In http://reviews.llvm.org/D12906#272243, @zaks.anna wrote:
> I think you misunderstood my comment. I am not talking about using the
> existing HTML files here but rather having an HTML viewer, which you could
> use to browse source code. This viewer would
honggyu.kim added a comment.
Hi Gabor,
I was away from the office for 3 weeks so I couldn't catch up the issues but
you almost completed the patch now.
Thanks for this work!
I just found a tiny problem and described it in the line comment so please have
a look at it.
Comment
honggyu.kim abandoned this revision.
honggyu.kim added a comment.
Hi Anna,
I was away from the office for 3 weeks. Sorry for the late answer.
In http://reviews.llvm.org/D12906#256215, @zaks.anna wrote:
> I find it very confusing to have 2 different fields for bug identification -
> issue_hash
honggyu.kim added a comment.
In http://reviews.llvm.org/D12906#250237, @zaks.anna wrote:
> I agree with Gabor and do not think we should have 2 entries in the plist
> file for bug identification. This is confusing and I do not see a need for
> this.
>
> Any comparison script can check for 2 fie
honggyu.kim added a comment.
Hi Babati,
Can you please rebase this patch based on http://reviews.llvm.org/D12673?
It is just a whitespace cleanup patch and I wrote
http://reviews.llvm.org/D12906 based on this patch after applying whitespace
cleanup at the end of each line.
http://reviews.llvm.o
honggyu.kim added a comment.
Hi Anna and Gábor,
I modified this patch based on http://reviews.llvm.org/D10305 following your
comments.
I think we can distinguish issue_hash and Bug ID. issue_hash is considered as a
subset of Bug ID here.
1. issue_hash contains
(1) column number
(2) source lin
honggyu.kim updated the summary for this revision.
honggyu.kim updated this revision to Diff 35070.
http://reviews.llvm.org/D12906
Files:
include/clang/StaticAnalyzer/Core/BugId.h
lib/StaticAnalyzer/Core/BugId.cpp
lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
lib/StaticAnalyzer/Core/PlistDi
honggyu.kim added a comment.
You made a comment while I was writing other comment :)
In http://reviews.llvm.org/D12906#248392, @zaks.anna wrote:
> This new patch does not seem to build on top of
> http://reviews.llvm.org/D10305 but is an alternative way of generating the
> hash that reuses a l
honggyu.kim added a comment.
Just to make it simpler to understand, original strings of issue_hash are as
below with this patch:
BUG 1
3$returna;$Garbage return value
BUG 2
3$intb=a;$Assigned value is garbage or undefined
BUG 3
3$returna;$Garbage return value
This consists of the 3 f
honggyu.kim added a comment.
I think we can enhance the bug comparison method one by one rather than making
a big change at once.
The point of this patch is to make "issue_hash" field in plist more robust
while still using existing CmpRuns.py for comparison.
I would like to hear any kind of com
honggyu.kim added a comment.
In http://reviews.llvm.org/D10305#242159, @zaks.anna wrote:
> honggyu.kim,
>
> Uniquing HTML reports is out of the scope of this patch and should be
> discussed elsewhere (either send a design idea to cfe-dev, send a patch for
> review, or file a bugzilla request).
honggyu.kim added a comment.
issue_hash is generated with hash value rather than line offset from function
as below:
issue_hash765b04830932fef5631bf3c48c4d2db2
http://reviews.llvm.org/D12906
___
cfe-commits mailing list
cfe-commits@lists.llvm.or
honggyu.kim created this revision.
honggyu.kim added reviewers: jordan_rose, krememek, zaks.anna, danielmarjamaki,
babati, dcoughlin.
honggyu.kim added subscribers: cfe-commits, phillip.power, seaneveson,
j.trofimovich, hk.kang, eszasip, dkrupp, o.gyorgy, xazax.hun, premalatha_mvs.
This patch br
honggyu.kim added a comment.
In http://reviews.llvm.org/D12827#244906, @ayartsev wrote:
> Thanks, Honggyu!
You're welcome, Антон!
http://reviews.llvm.org/D12827
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin
honggyu.kim updated the summary for this revision.
honggyu.kim updated this revision to Diff 34624.
http://reviews.llvm.org/D12827
Files:
tools/scan-build/scan-build
Index: tools/scan-build/scan-build
===
--- tools/scan-build/scan
honggyu.kim added a comment.
After applying http://reviews.llvm.org/rL247466, I got the following error
which was not shown before.
scan-build: error: Cannot find an executable 'clang' relative to scan-build.
Consider using --use-analyzer to pick a version of 'clang' to use for static
analy
honggyu.kim created this revision.
honggyu.kim added reviewers: ayartsev, zaks.anna, krememek.
honggyu.kim added subscribers: jordan_rose, dcoughlin, cfe-commits.
This patch fixes an error made by rL247466.
It mistakenly wrote "-d" check in if statement with the file name itself not a
directory.
honggyu.kim added a comment.
In http://reviews.llvm.org/D12673#241100, @dblaikie wrote:
> We generally don't do large whitespace changes like this because of issue
> with version control (not all told are whitespace ignorant which makes
> history work difficult).
I agree that it would be diff
honggyu.kim added a comment.
I would like to also write about bug identification methods.
As I observed the current CmpRuns.py script, the IssueIdentifier is defined as
follows:
def getIssueIdentifier(self) :
id = self.getFileName() + "+"
if 'issue_context' in self._data :
honggyu.kim added a comment.
Sorry for the late answer.
I would like to write the comment for HTML report comparison first.
> > As I mentioned, I think the simplist way of comparing HTML reports is to
> > have the BugID in each HTML report so that we can just compare HTML reports
> > >whether t
honggyu.kim added a comment.
In http://reviews.llvm.org/D12673#240933, @sylvestre.ledru wrote:
> Note: I am not the owner of this code but to me, this should be committed
> without a review.
I don't have commit permission, so please anyone commit it instead of me.
http://reviews.llvm.org/D12
honggyu.kim added a comment.
If anyone thinks this patch is too big, I can submit smaller separated patches.
http://reviews.llvm.org/D12673
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-co
honggyu.kim accepted this revision.
honggyu.kim added a reviewer: honggyu.kim.
honggyu.kim added a comment.
This revision is now accepted and ready to land.
In http://reviews.llvm.org/D10356#219772, @krememek wrote:
> Committed r244400
Thanks for accepting it!
http://reviews.llvm.org/D10356
honggyu.kim added a comment.
In http://reviews.llvm.org/D10305#221121, @zaks.anna wrote:
> honggyu.kim,
>
> You are right, CmpRuns.py does not work with HTML files.
>
> The list of HTML reports is produced by scan-build and there is no facility
> there to search for newly generated reports.
Th
honggyu.kim added a comment.
Hi Babati,
I'm sorry for the late appreciation response. I can see the Bug ID in HTML.
I have also tested CmpRuns.py and it works also fine as Anna explained.
CmpRuns.py works great with plist files, but I personally want to compare two
reports with HTML results with
Hi Anton,
Since D10356 is committed first, your patch(D6551) has to be modified based on
it.
So I just did some modification to resolve a conflict.
I was trying to upload the patch in phabricator, but I don't have a permission
to update your patch.
I just attached the modified version so that y
honggyu.kim added a comment.
In http://reviews.llvm.org/D10356#219499, @krememek wrote:
> I see. The problem is that the ad hoc interposition that scan-build uses
> does not handle the case that the underlying compiler is built as a
> cross-compiler, but does not take a direct target triple.
54 matches
Mail list logo
